Opened on 07/21/2017 at 02:35:06 PM
Closed on 08/28/2017 at 12:49:09 PM
Last modified on 09/24/2017 at 06:08:34 AM
#5438 closed change (fixed)
Listen to DOM changes to eventually reapply filters.
Reported by: | hfiguiere | Assignee: | hfiguiere |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Core | Keywords: | |
Cc: | fhd, trev, arthur | Blocked By: | #5395 |
Blocking: | #5000 | Platform: | Unknown / Cross platform |
Ready: | yes | Confidential: | no |
Tester: | Ross | Verified working: | yes |
Review URL(s): |
Description (last modified by trev)
Background
Currently, we only apply element hiding emulation filters once. Subsequent DOM modifications are mostly ignored, only new stylesheets are processed.
What to change
We need to listen to DOM modifications and reapply the filters as necessary. At the same time, we should avoid doing too much work. There seem to be four scenarios to consider here:
- A new subtree is added to the document: this is the obvious case, we have to apply all DOM-dependent rules on that subtree, just in case any of the elements there are matched. Here, "DOM-dependent rules" means all rules that aren't applied via selectors. In terms of our code that's pattern.selectors.some(s => s.preferHideWithSelector) && !pattern.selectors.some(s => s.requiresHiding).
- Some DOM modification (can be a new subtree but also attribute modifications for example) has side-effects on an -abp-has() pseudo of a rule. For example, -abp-has(.foo) wasn't matching for a node before but now a class="foo" attribute has been added on its child. In order to detect which nodes might be affected by a DOM change we need to keep track of all nodes we evaluated -abp-has() pseudos for. We can use a WeakMap for that - for each element that we evaluate this._innerSelectors on we store the reference to the remaining chain. So if the chain is [PlainSelector(#foo), HasSelector(.bar), PlainSelector(+ div)] then we should use the map to associate [HasSelector(.bar), PlainSelector(+ div)] with the element #foo. Note that we might have to associate multiple chains with the same element. For any DOM modifications later, we have to go through the parents of the modified element and check whether we find them in the map. If we do we need to re-evaluate the associated chains on these elements.
- Some DOM modification has side-effects on a -abp-properties() pseudo of a DOM-dependent rule. This could happen if -abp-properties() and -abp-has() are used in the same rule, e.g. #foo:-abp-has(.bar) + -abp-properties(background-color: red). If the class attribute for the next sibling of the #foo element changes and with it the background color, this rule might apply even though it didn't before. I think that we want to ignore this scenario for now.
- Some DOM modification has side-effects on a plain selector rule. For example, we have the selector #foo + .bar + .bas processed via element hiding emulation. If the class attribute for the next sibling of the #foo element changes to bar this rule might match some element even though it didn't before. I think that we want to ignore this scenario, with the support for plain selectors via element hiding emulation being a temporary hack that we only implemented because it was so easy.
Note
This require the changes for issue #5395 as we'll plug into the same logic.
Attachments (0)
Change History (9)
comment:1 Changed on 07/21/2017 at 07:54:00 PM by hfiguiere
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:2 Changed on 07/21/2017 at 09:32:44 PM by trev
- Description modified (diff)
- Priority changed from Unknown to P2
- Ready set
comment:3 Changed on 07/24/2017 at 09:16:31 PM by hfiguiere
- Blocking 5000 added
comment:4 Changed on 07/25/2017 at 09:01:17 AM by arthur
- Cc arthur added
comment:5 Changed on 08/08/2017 at 10:59:32 AM by trev
comment:6 Changed on 08/09/2017 at 08:17:15 PM by hfiguiere
Updated the patch.
comment:7 Changed on 08/28/2017 at 12:48:09 PM by abpbot
A commit referencing this issue has landed:
Issue 5438 - Observer DOM changes to reapply filters.
comment:8 Changed on 08/28/2017 at 12:49:09 PM by hfiguiere
- Resolution set to fixed
- Status changed from reviewing to closed
comment:9 Changed on 09/24/2017 at 06:08:34 AM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Hiding elements that match after DOM changes works in general. I've made a note to create test cases for the examples described above.
ABP 1.13.3.1838
Chrome 49 / 61 / Windows 10
Opera 39 / 47 / Windows 10
Felix and me discussed the approach here today. The trouble is, the approach I outlined so far is fairly complicated while still leaving lots of holes. In the end, we have to face the fact that with our set of rules a change to a DOM element might affect any other DOM element anywhere in the document. We cannot limit the work in a meaningful way.
So now the thought is: what if we reapply all filters on any DOM modification but make sure it happens in a non-blocking way? In detail, this should mean:
Hubert, what do you think?