Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Include ytd-video-masthead-ad to removeEmptyElements.css #7942

Closed
wants to merge 1 commit into from
Closed

Include ytd-video-masthead-ad to removeEmptyElements.css #7942

wants to merge 1 commit into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 29, 2017

Addresses #7818

Auditors: @bbondy @lukemulks

Test Plan:

  1. Open youtube.com in the experimental mode (try on the privade tab)
  2. Make sure you do not see the div placeholder
  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Addresses #7818

Auditors: @bbondy @lukemulks

Test Plan:
1. Open youtube.com in the experimental mode
2. Make sure you do not see the div placeholder
@lukemulks
Copy link
Collaborator

@luixxiul @bsclifton @bbondy

Confirming that I'm not seeing the placeholder div displaying in-page:
brave-yt-css-04072017-indom-dev
brave-yt-css-04072017-mastad-indom-dev

I am seeing the expanded state css class for the element, but the placeholder size (height and width) are not displaying that matches the element in the screenshot from the original issue (which was an empty container, expanded state, included a click to close ad button).

Confirming that the ad call for the 970x250 masthead ad is blocked

brave-yt-css-04072017-mastad-net-traffic-dev

I did check against the production version of the page, and I'm not seeing the placeholder there either.
This issue might be something only users outside of the US are experiencing, because I haven't been seeing the issue before the testing today.

We might want to have someone outside of the US check this to confirm the container that @luixxiul has been seeing is no longer displaying w/this fix so we can fully cover off.

@bbondy were you seeing the empty container from your geo?

If you are/were, would you mind taking a look to confirm?

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 8, 2017

This issue might be something only users outside of the US are experiencing

I rarely see the placeholder even in Japan. The top page might be experimental one.

@cndouglas
Copy link

cndouglas commented Apr 28, 2017

Should this be included in 0.15.2?

@luixxiul
Copy link
Contributor Author

luixxiul commented May 2, 2017

for me It is OK to close this. I no longer see the experimental page.

@bsclifton
Copy link
Member

Closing per comment by @luixxiul

@bsclifton bsclifton closed this May 2, 2017
@luixxiul luixxiul deleted the cssFiltering-youtube-followup branch May 21, 2017 06:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants