Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collapse HTML elements with blocked image/iframe requests #9144

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Collapse HTML elements with blocked image/iframe requests #9144

merged 4 commits into from
Jun 17, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Jun 15, 2021 •
edited
Loading

Resolves brave/brave-browser#14825
Resolves brave/brave-browser#14960

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Fix for brave/brave-browser#14825

Load the following HTML page from an HTTP or HTTPS source (i.e. http:// or https://, not a file:// URL):

<html>
    <body>
        <iframe src="https://1-1ads.com"></iframe>
        <iframe src="https://123freeavatars.com"></iframe>
        <iframe src="https://example.com"></iframe>
    </body>
</html>

With default Shields settings, only one frame (example.com) should be visible. The other two frames should be hidden.

With "Allow all trackers & ads" selected in the Shields panel, or with Shields disabled altogether, all three frames should be visible. Contents should load in the second frame (123freeavatars.com) and the third frame (example.com).

With default Shields settings, but with #brave-adblock-collapse-blocked-elements disabled in brave://flags, all three frames should be visible but only the last one (example.com) should actually load.

Fix for brave/brave-browser#14960

Visit https://www.twitch.tv/directory and observe profile and video thumbnail images loading correctly.

Add ||*^$image,domain=twitch.tv to the "Custom filters" section of brave://adblock and reload the Twitch page.

Observe that the profile and video thumbnail images are missing from the page, and that no "broken image" placeholders appear on the page.

Finally, disable #brave-adblock-collapse-blocked-elements in brave://flags and relaunch the browser. The missing images on the Twitch page should now have visible "broken image" placeholders like below:

broken image placeholder example.

@antonok-edm antonok-edm requested review from bridiver and iefremov June 15, 2021 19:04
@antonok-edm antonok-edm self-assigned this Jun 15, 2021
bridiver
bridiver reviewed Jun 15, 2021
View reviewed changes
browser/net/brave_proxying_url_loader_factory.cc Outdated
if (error_code == net::ERR_BLOCKED_BY_CLIENT) {
status.should_collapse_initiator = true;
}
OnRequestError(status);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should handle all of these in OnRequestError

@antonok-edm antonok-edm force-pushed the collapse-blocked-requests branch from 7d42ac4 to bcb592d Compare June 15, 2021 20:13
@antonok-edm antonok-edm requested a review from a team as a code owner June 15, 2021 20:40
@antonok-edm antonok-edm force-pushed the collapse-blocked-requests branch from 0e61c2d to e91bfd4 Compare June 15, 2021 21:00
@antonok-edm antonok-edm requested a review from bridiver June 15, 2021 21:08
@antonok-edm antonok-edm changed the title Collapse blocked requests Collapse HTML elements with blocked image/iframe requests Jun 15, 2021
mariospr
mariospr approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for chromium_src/

iefremov
iefremov reviewed Jun 16, 2021
View reviewed changes
browser/net/brave_proxying_url_loader_factory.cc
collapse_status.should_collapse_initiator = true;
}

target_client_->OnComplete(collapse_status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please add a test?

Copy link
Collaborator Author

@antonok-edm antonok-edm Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look into this briefly - collapsed elements cannot be detected in JS, and the only API Chromium exposes for this is HTMLImageElement::IsCollapsed() (and similar for iframes), which is pretty deep in Blink.

Is it allowed to access the Blink elements in browsertests? Otherwise we could probably try some hacky JS where we check the size of a parent element, but it seems fragile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried for a bit to access IsCollapsed, but I don't see any convenient way to do it. I've used the JS clientHeight property instead, which I've found works here.

@antonok-edm antonok-edm force-pushed the collapse-blocked-requests branch from c8e2530 to 5419b32 Compare June 16, 2021 20:06
@antonok-edm antonok-edm requested a review from iefremov June 16, 2021 20:09
iefremov
iefremov reviewed Jun 17, 2021
View reviewed changes
browser/brave_shields/ad_block_service_browsertest.cc Outdated
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

ASSERT_EQ(true, EvalJs(contents,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPECT_TRUE

iefremov
iefremov reviewed Jun 17, 2021
View reviewed changes
components/brave_shields/common/features.cc
@@ -15,6 +15,8 @@ namespace features {
// substituted for any canonical name found.
const base::Feature kBraveAdblockCnameUncloaking{
"BraveAdblockCnameUncloaking", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kBraveAdblockCollapseBlockedElements{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a comment?

iefremov
iefremov reviewed Jun 17, 2021
View reviewed changes
browser/brave_shields/ad_block_service_browsertest.cc

ASSERT_EQ(true, EvalJs(contents,
"let i = document.getElementsByClassName('adImage');"
"i[0].clientHeight === 0"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a comment

@antonok-edm antonok-edm merged commit f2f852b into master Jun 17, 2021
@antonok-edm antonok-edm deleted the collapse-blocked-requests branch June 17, 2021 15:49
@antonok-edm antonok-edm added this to the 1.28.x - Nightly milestone Jun 17, 2021
This was referenced Jun 22, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariospr mariospr mariospr approved these changes

@iefremov iefremov iefremov approved these changes

@bridiver bridiver Awaiting requested review from bridiver

Assignees

@antonok-edm antonok-edm

Labels
None yet
Projects
None yet
Milestone
1.28.x - Release
Development

Successfully merging this pull request may close these issues.

Collapse images with blocked content Collapse frames with blocked content
4 participants
@antonok-edm @bridiver @mariospr @iefremov