Issue 29770568: Issue 6645 - Collapse elements through user style sheets if possible (Closed)
|
Created:
May 4, 2018, 2:17 p.m. by Sebastian Noack Modified:
May 11, 2018, 12:36 p.m. Reviewers:
Manish Jethani CC:
Oleksandr, kzar Visibility:
Public. |
DescriptionIssue 6645 - Collapse elements through user style sheets if possible
Patch Set 1 #Patch Set 2 : Handle missing CSS.escape() on Microsoft Edge #Patch Set 3 : Cleaned up code #
Total comments: 7
Patch Set 4 : Addressed Manish's feedback #
Total comments: 5
Patch Set 5 : Rebased #Patch Set 6 : Got rif of hasSrc variable #MessagesTotal messages: 8
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:138: // the "visisiblity" CSS property. Typo: visibility https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:155: // Microsoft Edge does not support CSS.escape(). However, it doesn't Shouldn't this go before the above if block? https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:166: if (value && attr in element) Any reason why we're checking attr in element here?
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:138: // the "visisiblity" CSS property. On 2018/05/07 17:42:02, Manish Jethani wrote: > Typo: visibility Done. https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:155: // Microsoft Edge does not support CSS.escape(). However, it doesn't On 2018/05/07 17:42:02, Manish Jethani wrote: > Shouldn't this go before the above if block? Done. https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:166: if (value && attr in element) On 2018/05/07 17:42:02, Manish Jethani wrote: > Any reason why we're checking attr in element here? Yes, we only want to consider the src/srcset attribute if its actually supported by the element, but ignore it if its given in an element where it doesn't have any effect, e.g. <object src> or <iframe srcset>.
https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29772567/include.preload.js#... include.preload.js:166: if (value && attr in element) On 2018/05/09 16:30:46, Sebastian Noack wrote: > On 2018/05/07 17:42:02, Manish Jethani wrote: > > Any reason why we're checking attr in element here? > > Yes, we only want to consider the src/srcset attribute if its actually supported > by the element, but ignore it if its given in an element where it doesn't have > any effect, e.g. <object src> or <iframe srcset>. Acknowledged. https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... include.preload.js:163: for (let attr of ["src", "srcset"]) If I understand correctly [1], srcset is only relevant for <img>. In that case, the src attribute is treated as if it were part of srcset, and srcset can override the value of src with a more suitable image. We have two options: 1. Change the order here so that srcset takes precedence. This is problematic because we may still be blocking the URL specified in src, but we'll end up hiding based on srcset, and yet if the value of src changes to a non-blocked URL, the element will remain hidden. Similarly, the element will get displayed if the value of srcset changes, even though we've blocked the actual image URL based on src. 2. Hide based on both src and srcset (i.e. img[src="foo"][srcset="bar"]. Here we'll be hiding based on the "aggregate" of src and srcset so that if any of the values changes, the element gets displayed again. Arguably this is better, because if the image URL is blocked again after one of these values is updated, the element will get hidden again. What do you think? [1]: https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/...
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... include.preload.js:163: for (let attr of ["src", "srcset"]) On 2018/05/09 23:11:09, Manish Jethani wrote: > If I understand correctly [1], srcset is only relevant for <img>. In that case, > the src attribute is treated as if it were part of srcset, and srcset can > override the value of src with a more suitable image. > > We have two options: > > 1. Change the order here so that srcset takes precedence. This is problematic > because we may still be blocking the URL specified in src, but we'll end up > hiding based on srcset, and yet if the value of src changes to a non-blocked > URL, the element will remain hidden. Similarly, the element will get displayed > if the value of srcset changes, even though we've blocked the actual image URL > based on src. > > 2. Hide based on both src and srcset (i.e. img[src="foo"][srcset="bar"]. Here > we'll be hiding based on the "aggregate" of src and srcset so that if any of the > values changes, the element gets displayed again. Arguably this is better, > because if the image URL is blocked again after one of these values is updated, > the element will get hidden again. > > What do you think? > > [1]: > https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/... #2 is what my current implementation does. Also consider: If anything changes that triggers another filter hit and therefore another error event, we just add another selector to the style sheet with the current implementation.
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... include.preload.js:162: let hasSrc = false; Only one small nit, and this is totally a nit. We could avoid the hasSrc variable here by setting selector to "" initially, then later: if (selector) return element.localName + selector; return null; But anyway I don't care about this. LGTM https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... include.preload.js:163: for (let attr of ["src", "srcset"]) On 2018/05/10 01:08:22, Sebastian Noack wrote: > On 2018/05/09 23:11:09, Manish Jethani wrote: > > If I understand correctly [1], srcset is only relevant for <img>. In that > case, > > the src attribute is treated as if it were part of srcset, and srcset can > > override the value of src with a more suitable image. > > > > We have two options: > > > > 1. Change the order here so that srcset takes precedence. This is > problematic > > because we may still be blocking the URL specified in src, but we'll end up > > hiding based on srcset, and yet if the value of src changes to a non-blocked > > URL, the element will remain hidden. Similarly, the element will get displayed > > if the value of srcset changes, even though we've blocked the actual image URL > > based on src. > > > > 2. Hide based on both src and srcset (i.e. img[src="foo"][srcset="bar"]. > Here > > we'll be hiding based on the "aggregate" of src and srcset so that if any of > the > > values changes, the element gets displayed again. Arguably this is better, > > because if the image URL is blocked again after one of these values is > updated, > > the element will get hidden again. > > > > What do you think? > > > > [1]: > > > https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/... > > #2 is what my current implementation does. Also consider: If anything changes > that triggers another filter hit and therefore another error event, we just add > another selector to the style sheet with the current implementation. Ah, I didn't realize you were not breaking from the loop when a selector was added.
https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... include.preload.js:162: let hasSrc = false; On 2018/05/10 01:57:17, Manish Jethani wrote: > Only one small nit, and this is totally a nit. We could avoid the hasSrc > variable here by setting selector to "" initially, then later: > > if (selector) > return element.localName + selector; > > return null; Done.
On 2018/05/10 11:45:34, Sebastian Noack wrote: > https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js > File include.preload.js (right): > > https://codereview.adblockplus.org/29770568/diff/29775600/include.preload.js#... > include.preload.js:162: let hasSrc = false; > On 2018/05/10 01:57:17, Manish Jethani wrote: > > Only one small nit, and this is totally a nit. We could avoid the hasSrc > > variable here by setting selector to "" initially, then later: > > > > if (selector) > > return element.localName + selector; > > > > return null; > > Done. LGTM 
|
