Issue 29670575: Issue 5899 - Use CSS attribute selectors for collapsing media elements (Closed)
|
Created:
Jan. 16, 2018, 10:28 a.m. by Manish Jethani Modified:
April 20, 2018, 11:05 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase #Patch Set 3 : Use CSS attribute selectors for collapsing #
Total comments: 15
Patch Set 4 : Address comments to Patch Set 3 #Patch Set 5 : Only append new selectors #
Total comments: 2
Patch Set 6 : Do not trace collapsing selectors #
Total comments: 3
Patch Set 7 : Check for "collapsing" group name #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Compare child name directly #MessagesTotal messages: 20
Patch Set 1 So far we have been using the same element hiding functionality for "collapsing" of elements when their source URLs are blocked. I don't think this is right. We should have a separate function for collapsing elements, and we should restore them back to their original display settings once their source URLs have changed. The current issue is that a website shows a preroll ad in a VIDEO element, then we "collapse" the element, then it switches the VIDEO element's SRC attribute to a different URL that is not blocked by any filter, but the VIDEO element remains hidden (sound still plays in the background!) because we never bother restoring it. This is a tentative fix and I'm doing it only for VIDEO and AUDIO elements. Please see comments inline. (Also I think we could switch to the other solution I mentioned in the issue comments, i.e. video[src="..."], but that'll have to wait until we start maintaining multiple style sheets in the content script, see review #29575739). https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:38: let collapsedElements = new WeakMap(); A weak map of collapsed elements to their original display settings. https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:136: function collapseElement(element) This is basically what hideElement does, except it's simpler. We don't need to set up a MutationObserver for collapsed elements. If a website insists on displaying an element that we have collapsed, it's up to them, since we are blocking the URL anyway. The hiding of the element here is more of a cosmetic touch, it does not affect ad blocking if the hiding is overridden. https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:150: if (display) Restore the original display settings. https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:190: let urls = getURLsFromElement(element, mediatype == "MEDIA"); We have to pass true here as the second argument, because media elements can have blob URLs, data URLs, and so on. We should restore the element if the URLs has changed to any one of these types as well. (Actually the page mentioned in the original issue report has this situation.) https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:206: if (collapse) I'm distinguishing "collapsing" from "hiding" here. I think it's different: collapsing should not be so aggressive (no need to set up a MutationObserver), and it should restore the original display settings when the URL changes to something that isn't blocked. Hiding on the other hand is based on hiding filters and it should remain the way it is. So I think we should consider doing the "collapsing" for all types, not just media. https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:559: document.addEventListener("loadstart", event => The "loadstart" event is dispatched by VIDEO and AUDIO elements when the URL begins to load. We do get an "error" event afterwards after the URL is blocked by Adblock Plus. It means now we end up collapsing the element even before the URL has been blocked, but that's OK, it doesn't make much of a difference. This is mainly for restoring the element since we won't be getting an "error" event if the URL isn't blocked. https://codereview.adblockplus.org/29670575/diff/29670576/include.preload.js#... include.preload.js:562: if (typeMap.get(element.localName) == "MEDIA") We have to do this check because IMG and PICTURE elements also dispatch a "loadstart" event.
On 2018/01/16 10:47:27, Manish Jethani wrote: > (Also I think we could switch to the other solution I mentioned in the issue > comments, i.e. video[src="..."], but that'll have to wait until we start > maintaining multiple style sheets in the content script, see review #29575739). In that case should I review this one, or instead wait for the changes in the other review to land?
Changed the implementation now to use CSS attribute selectors. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:164: elemhide.addSelectors(Array.from(collapsingSelectors), null, "collapsing"); There's a new "collapsing" group for these selectors. Ideally we would have an append-only mode for the collapsing selectors since they are never removed. This would make it more efficient. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:209: let srcValue = collapsible ? element.getAttribute("src") : null; If the pre-roll ad is blocked then pretty quickly the page continues on to the actual video content, so we need to make sure we get the original value for the pre-roll before it is changed. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:500: addSelectors(selectors, filters, groupName = "emulated") Using a default value for groupName here (instead of setting it in the body or repeating it).
I wonder if the code would be easier to grok if you split isMediaElement out from isCollapsibleMediaElement but then combined collapseMediaElement with hideElement, so that we just call hideElement but then that decides these details of how it should be hidden. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:38: let collapsingSelectors = new Set(); I guess we'll slowly leak memory here, since we never remove the selectors again. I wonder if it matters and if there's something we can do to avoid that? https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:156: let selector = srcValue ? Nit: It would be clearer to do this IMO: if (!srcValue) return; let selector = element.localName + "[src=" + CSS.escape(srcValue) + "]"; ... if (!collapsingSelectors.has(selector)) https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:157: element.localName + "[src=" + CSS.escape(srcValue) + "]" : Maybe a dumb question but could this end up matching other elements too? https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:205: let collapsible = isCollapsibleMediaElement(element, mediatype); IMO this variable name is misleading since non-media elements are collapsible too right? https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:209: let srcValue = collapsible ? element.getAttribute("src") : null; On 2018/02/22 06:55:06, Manish Jethani wrote: > If the pre-roll ad is blocked then pretty quickly the page continues on to the > actual video content, so we need to make sure we get the original value for the > pre-roll before it is changed. Acknowledged. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:500: addSelectors(selectors, filters, groupName = "emulated") On 2018/02/22 06:55:06, Manish Jethani wrote: > Using a default value for groupName here (instead of setting it in the body or > repeating it). Acknowledged.
Patch Set 3 On 2018/02/26 17:18:59, kzar wrote: > I wonder if the code would be easier to grok if you split isMediaElement out > from isCollapsibleMediaElement but then combined collapseMediaElement with > hideElement, so that we just call hideElement but then that decides these > details of how it should be hidden. > hideElement is used for ElemHideEmulation mainly, and using it for collapsing is almost a hack (at least that's what it looks like to me). I prefer to leave that function alone and slowly try to use collapseMediaElement for all kinds of collapsing, not just media (renaming it to just collapseElement of course). But let me see if I can move things around a little bit. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:38: let collapsingSelectors = new Set(); On 2018/02/26 17:18:58, kzar wrote: > I guess we'll slowly leak memory here, since we never remove the selectors > again. I wonder if it matters and if there's something we can do to avoid that? I wouldn't call that "leaking memory" since we do need the selectors to stay in the set (i.e. we don't have a case for removing any selector, and if we did we would do it). https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:156: let selector = srcValue ? On 2018/02/26 17:18:58, kzar wrote: > Nit: It would be clearer to do this IMO: > > if (!srcValue) > return; > > let selector = element.localName + "[src=" + CSS.escape(srcValue) + "]"; > > ... > > if (!collapsingSelectors.has(selector)) Ah, you're right. Done. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:157: element.localName + "[src=" + CSS.escape(srcValue) + "]" : On 2018/02/26 17:18:58, kzar wrote: > Maybe a dumb question but could this end up matching other elements too? No, that's a good question. In isCollapsibleMediaElement we're checking if the element is a media element (<video> or <audio>), if it has an src property, and if it has no child elements <source> or <track>. If all of these are true, then it's collapsible. Why? Because getURLsFromMediaElement will return only the value of the src property in this case, so we can be certain that whatever element we're collapsing is based solely on the value of that property. Since the src property in JS reflects the value of the src attribute in HTML, we can hide the element based on the value of src attribute using a CSS attribute selector. So basically if there are multiple media elements on the page with the same src attribute value, then they will all be hidden (even if their media hasn't started loading yet!), and this is a good thing. We do want to hide those. https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:205: let collapsible = isCollapsibleMediaElement(element, mediatype); On 2018/02/26 17:18:58, kzar wrote: > IMO this variable name is misleading since non-media elements are collapsible > too right? Yeah, I suppose you're right. Updated.
On 2018/02/27 13:11:49, Manish Jethani wrote: > Patch Set 3 Uh ... I mean Patch Set 4.
https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:164: elemhide.addSelectors(Array.from(collapsingSelectors), null, "collapsing"); On 2018/02/22 06:55:06, Manish Jethani wrote: > There's a new "collapsing" group for these selectors. > > Ideally we would have an append-only mode for the collapsing selectors since > they are never removed. This would make it more efficient. Good idea, let's do that now?
Patch Set 5: Only append new selectors https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29705555/include.preload.js#... include.preload.js:164: elemhide.addSelectors(Array.from(collapsingSelectors), null, "collapsing"); On 2018/02/27 14:18:38, kzar wrote: > On 2018/02/22 06:55:06, Manish Jethani wrote: > > There's a new "collapsing" group for these selectors. > > > > Ideally we would have an append-only mode for the collapsing selectors since > > they are never removed. This would make it more efficient. > > Good idea, let's do that now? Done. https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js... lib/cssInjection.js:104: let styleSheet = ""; I changed this to an empty string because it doesn't make a difference to the rest of the code but string concatenation works as expected.
LGTM, you want to take a look Sebastian? https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29670575/diff/29710581/lib/cssInjection.js... lib/cssInjection.js:104: let styleSheet = ""; On 2018/02/27 16:00:22, Manish Jethani wrote: > I changed this to an empty string because it doesn't make a difference to the > rest of the code but string concatenation works as expected. Acknowledged.
Patch Set 6: Do not trace collapsing selectors https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#... include.preload.js:527: // Only trace selectors that are based directly on hiding filters I forgot to add this. We do not want to trace collapsing selectors, unless I'm mistaken.
https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#... include.preload.js:527: // Only trace selectors that are based directly on hiding filters On 2018/02/28 10:31:15, Manish Jethani wrote: > I forgot to add this. We do not want to trace collapsing selectors, unless I'm > mistaken. You're right, but how about `groupName != "collapse"`? (Or whatever the group name for the collapsing selectors is called.)
Patch Set 7: Check for "collapsing" group name https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29711587/include.preload.js#... include.preload.js:527: // Only trace selectors that are based directly on hiding filters On 2018/03/02 09:37:28, kzar wrote: > On 2018/02/28 10:31:15, Manish Jethani wrote: > > I forgot to add this. We do not want to trace collapsing selectors, unless I'm > > mistaken. > > You're right, but how about `groupName != "collapse"`? (Or whatever the group > name for the collapsing selectors is called.) Done.
Let me know if I miss something, but from a first glance at the latest patch set, it seems with this change we will merely use user style sheets to collapse <video> and <audio> element that have a "src" attribute. Is my understanding correct? And if so, why would you want to do that? Media elements blocked by Adblock Plus are rather rare (compared to images and iframes). So specifically only collapsing media elements using user style sheets seems kinda pointless to me.
On 2018/03/10 23:43:49, Sebastian Noack wrote: > Let me know if I miss something, but from a first glance at the latest patch > set, it seems with this change we will merely use user style sheets to collapse > <video> and <audio> element that have a "src" attribute. > > Is my understanding correct? And if so, why would you want to do that? Media > elements blocked by Adblock Plus are rather rare (compared to images and > iframes). So specifically only collapsing media elements using user style sheets > seems kinda pointless to me. See the description of Trac #5899. It's a bug that the video starts to play, you hear the sound but you can't see the video, thanks to ABP's aggressive collapsing by setting the style attribute. So I think the point is to avoid that situation first of all. Secondly I think we want to avoid modifying the DOM as much as we can, so for cases where it's possible to use CSS selectors we should go ahead and do it. Unfortunately it's not possible in all cases, e.g. here if the <video> element has <source> or <track> children. I'm going to investigate separately about iframes and images but I'm guessing we could use this strategy there too (I just wanted to fix the actual bug first).
On 2018/03/11 00:24:34, Manish Jethani wrote: > So I think the point is to avoid that situation first of all. Secondly I think > we want to avoid modifying the DOM as much as we can, so for cases where it's > possible to use CSS selectors we should go ahead and do it. How are these issues related? As far as I understand, using user style sheets for element collapsing (not only for media elements), and preventing collapsed media elements to play in the background, are two separate issues, that should be addressed separately. Or do I miss something?
On 2018/03/11 01:14:56, Sebastian Noack wrote: > On 2018/03/11 00:24:34, Manish Jethani wrote: > > So I think the point is to avoid that situation first of all. Secondly I think > > we want to avoid modifying the DOM as much as we can, so for cases where it's > > possible to use CSS selectors we should go ahead and do it. > > How are these issues related? As far as I understand, using user style sheets > for element collapsing (not only for media elements), and preventing collapsed > media elements to play in the background, are two separate issues, that should > be addressed separately. Or do I miss something? It just so happens that the correct fix for the issue is to change the way elements are collapsed. You can see my original fix in Patch Set 1 here, but we decided that wasn't the best approach. Also see my comment here: https://issues.adblockplus.org/ticket/5899#comment:11 We are not "preventing collapsed media elements to play", we are letting them play and letting them be displayed again (since the value of the src attribute has changed, i.e. it is not a blocked URL anymore). Both Adguard and uBlock Origin seems to be using CSS attribute selectors to collapse media elements. Do you have any ideas about a better way to address this issue? The problem is that we are hiding the element when the pre-roll ad is blocked, but then we never show it again when the actual video content starts playing.
Otherwise this LGTM. I read through your discussion and I just wanted to add that I think in practice this change could help solve a real problem which some of our users have, and like Manish points out it seems we don't have any better ideas how we can achieve the same result. https://codereview.adblockplus.org/29670575/diff/29713555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29713555/include.preload.js#... include.preload.js:147: if (["source", "track"].includes(child.localName)) Nit: It seems kinda wasteful to create the array, especially inside the loop, just to check if the localName is included. I'd rather something like this, but I don't insist: if (child.localName == "source" || child.localName == "track")
Patch Set 8: Rebase Patch Set 9: Compare child name directly https://codereview.adblockplus.org/29670575/diff/29713555/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29670575/diff/29713555/include.preload.js#... include.preload.js:147: if (["source", "track"].includes(child.localName)) On 2018/04/18 14:56:13, kzar wrote: > Nit: It seems kinda wasteful to create the array, especially inside the loop, > just to check if the localName is included. I'd rather something like this, but > I don't insist: > > if (child.localName == "source" || child.localName == "track") Done.
Extra LGTM 
|
