Issue 5088751004942336: Issue 370 - Right-clicked element is removed independent of created filter (Closed)
|
Created:
Jan. 10, 2014, 10:27 a.m. by Thomas Greiner Modified:
April 22, 2015, 9:04 a.m. Visibility:
Public. |
DescriptionSee https://adblockplus.org/forum/viewtopic.php?f=11&t=20408
I reused should-collapse to check whether the element should be removed or not.
In addition to fixing this issue I also discovered an oversight from the Safari merge (|request| variable was renamed to |msg| but not all references were updated) which I corrected in this review.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebase to rev 3c9cea80c481 #Patch Set 3 : #
Total comments: 7
MessagesTotal messages: 14
LGTM
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/bloc... File block.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/bloc... block.js:83: filters: filters Why send the filters and have the other end ignore that parameter? IMHO all the changes in this file are unnecessary. Also, respecting the remove parameter in the content script makes sense to avoid wasting resources. http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:578: if (clickHideFiltersDialog) You should add |&& currentElement| check here which was performed by the original code - it might have been unset somewhere else already. http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); What if an element hiding filter was added? Not only will typeMap[element.localName] be bogus then, the element won't match any filters - even if it has a URL.
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); The problem is not (only) that the element is removed when the filter was edited and doesn't match currentElement anymore. What is if an other element matches after the filter was edited? What is if the filter matches multiple elements? Also the wrong elements are highlighted when the filter has been edited. So what we actually should do, is updating the highlighted elements as soon as the filter is edited. And finally removing the highlighted elements instead currentElement.
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); On 2014/06/25 16:40:59, Sebastian Noack wrote: > What is if an other element matches after the filter was edited? We don't care, going through the entire document looking for matching elements would go too far. We merely want to give immediate feedback - and we do it by removing the element that the user selected, *if* it actually should be removed of course.
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); On 2014/06/25 18:49:34, Wladimir Palant wrote: > On 2014/06/25 16:40:59, Sebastian Noack wrote: > > What is if an other element matches after the filter was edited? > > We don't care, going through the entire document looking for matching elements > would go too far. Actually we exactly do that already, when the matching elements are highlighted when the dialog pops up. See highlightElements(). > We merely want to give immediate feedback - and we do it by > removing the element that the user selected, *if* it actually should be removed > of course. IMO users would rather expect all matching elements to be removed immediately. Alternatively, we could call the button "Save and reload page" and doing so, which would also have immediate effect, fix this issue and lets us eliminate some code.
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/bloc... File block.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/bloc... block.js:83: filters: filters On 2014/01/10 16:28:56, Wladimir Palant wrote: > Why send the filters and have the other end ignore that parameter? IMHO all the > changes in this file are unnecessary. Also, respecting the remove parameter in > the content script makes sense to avoid wasting resources. You're right. This is code from earlier approaches. http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); This issue only addresses the case when a filter doesn't match but the selected element getting removed regardless. I can prepend a check that tests whether an element hiding filter matches the selected element. Any further changes should be discussed separately because if we only want to allow removal of the selected element then a text input field is clearly the wrong UI element. So either we allow the input of any kind of filters and show the results to the user or we limit its capabilities e.g. by changing it to checkboxes.
http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/5629499534213120/incl... include.postload.js:591: }); On 2014/07/17 15:35:49, Thomas Greiner wrote: > This issue only addresses the case when a filter doesn't match but the selected > element getting removed regardless. I can prepend a check that tests whether an > element hiding filter matches the selected element. That would be the best solution. If that's too complicated we could continue always removing the element if an element hiding filter is added.
The element hiding filter check is now in. Additionally, it was quite easy to apply the element hiding filters directly to the page so that they can also affect other elements that match the filters. Another change to the previous approach is that the selected element is no longer being removed but only hidden when it matches a blocking filter (see https://hg.adblockplus.org/adblockpluschrome/rev/bfedb22e016e).
Sorry about this review taking so long, I actually had it hanging there partially reviewed :-( http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/bloc... File block.js (right): http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/bloc... block.js:27: $("#cancelButton").click(closeDialog); closeDialog() still has a filters parameter, with your change it will get an Event object there. http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:574: setElemhideCSSRules(selectors); The code above has multiple issues: * It doesn't consider domain restrictions of element hiding rules. * It doesn't consider existing whitelisting rules. * It uses the setElemhideCSSRules function to insert a second stylesheet and potentially create a second shadow root, something that is known to have side-effects like resetting all CSS transitions and such. After thinking through it, the best solution seems to be sending a new "get-selectors" message and updating the stylesheet we have in this document already (which might not exist so we'll have to create one, but some side-effects in this rare scenario should be ok). http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:587: element.style.setProperty("display", "none", "important"); We don't need to check the element's parent node if we aren't removing it. Either way, I think that calling checkCollapse() here makes more sense than duplicating the logic (that function also treats frames correctly).
http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:587: element.style.setProperty("display", "none", "important"); On 2014/11/17 19:53:57, Wladimir Palant wrote: > We don't need to check the element's parent node if we aren't removing it. > Either way, I think that calling checkCollapse() here makes more sense than > duplicating the logic (that function also treats frames correctly). What is if element collapsing is disabled in the options? Currently the element is removed regardless, which is a bug (#1626). The behavior with this patch would be that the element keeps showing, which isn't any better. How about cloning the element and replacing the element with its clone (in case a blocking filter were added), that it gets blocked via the webRequest API, but only collapsed if supposed to?
http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:574: setElemhideCSSRules(selectors); On 2014/11/17 19:53:57, Wladimir Palant wrote: > The code above has multiple issues: > > * It doesn't consider domain restrictions of element hiding rules. > * It doesn't consider existing whitelisting rules. > * It uses the setElemhideCSSRules function to insert a second stylesheet and > potentially create a second shadow root, something that is known to have > side-effects like resetting all CSS transitions and such. > > After thinking through it, the best solution seems to be sending a new > "get-selectors" message and updating the stylesheet we have in this document > already (which might not exist so we'll have to create one, but some > side-effects in this rare scenario should be ok). What we could do is instead of just forwarding the "clickhide-close" message through the background page we could reuse the same logic used for "get-selectors" to filter them right there and only return the ones to the content script that apply. http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:587: element.style.setProperty("display", "none", "important"); On 2014/11/27 12:43:40, Sebastian Noack wrote: > On 2014/11/17 19:53:57, Wladimir Palant wrote: > > We don't need to check the element's parent node if we aren't removing it. > > Either way, I think that calling checkCollapse() here makes more sense than > > duplicating the logic (that function also treats frames correctly). > > What is if element collapsing is disabled in the options? Currently the element > is removed regardless, which is a bug (#1626). The behavior with this patch > would be that the element keeps showing, which isn't any better. > > How about cloning the element and replacing the element with its clone (in case > a blocking filter were added), that it gets blocked via the webRequest API, but > only collapsed if supposed to? That's a valid comment and it doesn't seem like we can find a non-hacky solution for this. To accomplish to show the placeholder, I'd still prefer to simply change the URL of the element though. Personally, I'd rather remove the "remove placeholders" option from the options page since it causes so much confusion for the user and unnecessary logic in this case. But that's a discussion that should be held outside of this review, of course.
http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... File include.postload.js (right): http://codereview.adblockplus.org/5088751004942336/diff/6313034989436928/incl... include.postload.js:587: element.style.setProperty("display", "none", "important"); On 2014/11/27 14:46:07, Thomas Greiner wrote: > On 2014/11/27 12:43:40, Sebastian Noack wrote: > > On 2014/11/17 19:53:57, Wladimir Palant wrote: > > > We don't need to check the element's parent node if we aren't removing it. > > > Either way, I think that calling checkCollapse() here makes more sense than > > > duplicating the logic (that function also treats frames correctly). > > > > What is if element collapsing is disabled in the options? Currently the > element > > is removed regardless, which is a bug (#1626). The behavior with this patch > > would be that the element keeps showing, which isn't any better. > > > > How about cloning the element and replacing the element with its clone (in > case > > a blocking filter were added), that it gets blocked via the webRequest API, > but > > only collapsed if supposed to? > > That's a valid comment and it doesn't seem like we can find a non-hacky solution > for this. To accomplish to show the placeholder, I'd still prefer to simply > change the URL of the element though. If that is sufficient, fine for me. > Personally, I'd rather remove the "remove placeholders" option from the options > page since it causes so much confusion for the user and unnecessary logic in > this case. But that's a discussion that should be held outside of this review, > of course. That's what I thought. But then I remembered that there is also a filter list option to disable element collapsing. :/
Feel free to close or delete this review now. My patch has been merged instead: http://codereview.adblockplus.org/5838948538515456 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
