Issue 5838948538515456: Issue 370 - Make "Block element" hide elements for added filters (Closed)
|
Created:
March 4, 2015, 10:07 a.m. by Sebastian Noack Modified:
March 4, 2015, 9:59 p.m. CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 370 - Make "Block element" hide elements for added filters
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Added comments #
Total comments: 3
Patch Set 3 : Restored failsafe code #Patch Set 4 : Fixed failsafe code #
Total comments: 2
Patch Set 5 : Improved comment #Patch Set 6 : Replace existing rules #
Total comments: 5
Patch Set 7 : Addressed comments and simplified loop #MessagesTotal messages: 19
I think this is a cool idea but I think with blocking rules it will be a step backwards from a UX perspective. (Especially important now that the block element tool only doesn't bother to generate hiding rules if a blocking rule has been found.) I'm not exactly sure what the solution should be, I just think it kind of sucks from the user's point of view if you right click to block something and when you click OK it doesn't vanish. It will seem broken. (I agree that it also seems broken how it works currently that if you delete all the rules and click OK that the element you clicked on vanishes.) http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; Shouldn't we check msg.remove is true first?
Maybe we should vanish the element user right clicked on if there are no element hiding rules returned, use the element hiding rules if there are some returned and if there are no rules generated at all do nothing? (Just an idea and I realise it's technically wrong... but from the user's point of view maybe it will seem less broken.)
(Or maybe if there are blocking rules generated we should somehow notify the user that they need to refresh the page for them to take effect?)
Blocking rules are still considered, though in a limited way. After the filters were added, the code checks whether the element initially selected, matches any blocking filter, and hides it if it does. Only in the case when generated filters were modified to block an other element than the one the user has initially selected, nothing would happen. However, this is still slightly better than the current behavior, where the initially selected element is removed regardless whether it matches any of the added filters.
http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 11:04:02, kzar wrote: > Shouldn't we check msg.remove is true first? Well, we do above. ;)
Oh cool, glad to hear it. In that case I think your approach is fine, it will do what users expect as far as is reasonably possible. http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:724: checkCollapse(currentElement.prisoner || currentElement); IMHO there should be a comment here to mention we're hiding the right clicked upon element if a blocking rule now matches it. http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 11:34:52, Sebastian Noack wrote: > On 2015/03/04 11:04:02, kzar wrote: > > Shouldn't we check msg.remove is true first? > > Well, we do above. ;) Gah, sorry! http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; I think there should also be a comment here to say that selectors are returned for any new hiding rules we added and so we need to hide matched elements for those.
Other changes due to rebase http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 13:09:03, kzar wrote: > I think there should also be a comment here to say that selectors are returned > for any new hiding rules we added and so we need to hide matched elements for > those. I don't think it needs to be that verbose. But the other case might need some more explanation.
LGTM http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... File include.postload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5717271485874176/incl... include.postload.js:726: var selectors = msg.selectors; On 2015/03/04 14:02:01, Sebastian Noack wrote: > On 2015/03/04 13:09:03, kzar wrote: > > I think there should also be a comment here to say that selectors are returned > > for any new hiding rules we added and so we need to hide matched elements for > > those. > > I don't think it needs to be that verbose. But the other case might need some > more explanation. Looks great.
Other than the issue below, the changes look fine to me. http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... File include.preload.js (left): http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... include.preload.js:226: setTimeout(setRules, 0); Why did you remove this failsafe? It was introduced in https://hg.adblockplus.org/adblockpluschrome/rev/191f69afc9d6, is it obsolete now? I couldn't reproduce the scenario in question in Chrome 41 so maybe it was indeed fixed there. But what about Chrome 28 that we are still supporting?
http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... File include.preload.js (left): http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... include.preload.js:226: setTimeout(setRules, 0); On 2015/03/04 14:45:54, Wladimir Palant wrote: > Why did you remove this failsafe? It was introduced in > https://hg.adblockplus.org/adblockpluschrome/rev/191f69afc9d6, is it obsolete > now? I couldn't reproduce the scenario in question in Chrome 41 so maybe it was > indeed fixed there. But what about Chrome 28 that we are still supporting? First I didn't remember why I added that code, and couldn't reproduce anything like that anymore. However, I remembered again, it happens and still does for example on http://www.chip.de/News_29559209.html at least with EasyList Germany + Acceptable Ads.
http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... File include.preload.js (left): http://codereview.adblockplus.org/5838948538515456/diff/5673385510043648/incl... include.preload.js:226: setTimeout(setRules, 0); On 2015/03/04 15:10:22, Sebastian Noack wrote: > On 2015/03/04 14:45:54, Wladimir Palant wrote: > > Why did you remove this failsafe? It was introduced in > > https://hg.adblockplus.org/adblockpluschrome/rev/191f69afc9d6, is it obsolete > > now? I couldn't reproduce the scenario in question in Chrome 41 so maybe it > was > > indeed fixed there. But what about Chrome 28 that we are still supporting? > > First I didn't remember why I added that code, and couldn't reproduce anything > like that anymore. However, I remembered again, it happens and still does for > example on http://www.chip.de/News_29559209.html at least with EasyList Germany > + Acceptable Ads. I figured out the issue here. If adding a <style> to the shadow DOM of a subframe that navigated to another document while we waited for the response of the background page, the sheet attribute stays null. This means that our previous failsafe code was the wrong approach, since it kept rescheduling a timeout forever. Instead we should just bail out in that case.
http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/incl... File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/incl... include.preload.js:214: // property stays null, after adding the <style> element to the shadow DOM. Sorry about nit picking but let's reformulate that comment a bit to make the logic easier to follow: It can happen that the frame already navigated to a different location while we were waiting for the background page to respond. In that case our shadow root won't be in a document any more and style.sheet will consequently be null.
http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/incl... File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5709068098338816/incl... include.preload.js:214: // property stays null, after adding the <style> element to the shadow DOM. On 2015/03/04 19:06:15, Wladimir Palant wrote: > Sorry about nit picking but let's reformulate that comment a bit to make the > logic easier to follow: > > It can happen that the frame already navigated to a different location while we > were waiting for the background page to respond. In that case our shadow root > won't be in a document any more and style.sheet will consequently be null. I changed following: * I prefer to talk about a different document rather than a location the frame navigated to, since the location doesn't necessarily change, e.g. when relaoding or if the location is as bogus as "about:blank". * I actually didn't investigate what happened to the shadow root. I only know that when adding style elements there, the sheet property won't change. (However, it does when adding the style to an element in the light DOM). Point is, you shouldn't consider assumptions you didn't verify as facts. So I rather stick to my observations.
LGTM
I've uploaded a new patch set, that instead of applying the added element hiding filters, updates the whole stylesheet. This is what Wladimir's initial suggested. The complexity is about the same. The advantage however is that the result is more accurate, e.g. added whitelisting filters will be considered and added element hiding filters will respect whitelisting filters. What do you think?
Actually, I also like that variant better - it's just less of a hack than the other one. Looks good, merely two nits. http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/bloc... File block.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/bloc... block.js:57: closeDialog(false); Nit: This change is unrelated. LGTM for pushing it in a separate commit. http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/incl... File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/incl... include.preload.js:230: // the shadow DOM. Nit: this comment exceeds 80 character boundary now.
http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/bloc... File block.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/bloc... block.js:57: closeDialog(false); On 2015/03/04 21:26:14, Wladimir Palant wrote: > Nit: This change is unrelated. LGTM for pushing it in a separate commit. Yep, right. Leftover from earlier version of the patchset. http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/incl... File include.preload.js (right): http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/incl... include.preload.js:230: // the shadow DOM. On 2015/03/04 21:26:14, Wladimir Palant wrote: > Nit: this comment exceeds 80 character boundary now. Yeah, there is one more indentation level now. http://codereview.adblockplus.org/5838948538515456/diff/5743114304094208/incl... include.preload.js:241: while (selectors.length > 0) I reverted the loop logic to the simpler one, we had before, since we don't need to consider the case of already existing rules anymore.
LGTM 
|
