Opened on 02/27/2018 at 01:41:57 PM
Closed on 04/09/2019 at 07:14:26 PM
Last modified on 07/26/2019 at 03:12:34 AM
#6428 closed change (fixed)
Have generateStyleSheetForDomain provide exception filters if requested
Reported by: | kzar | Assignee: | jsonesen |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Core | Keywords: | |
Cc: | sergz, sebastian, jsonesen, mjethani, tjansen, sporz | Blocked By: | |
Blocking: | #6427 | Platform: | Unknown / Cross platform |
Ready: | yes | Confidential: | no |
Tester: | Ross | Verified working: | yes |
Review URL(s): |
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/24 |
Description (last modified by mjethani)
Background
With #6427 we'd like to start listing element hiding whitelisting filters which applied to the page in our developer tools panel. To do that we need the list of those whitelisting filters.
What to change
Modify generateStyleSheetForDomain in lib/elemHide.js to provide exception filters which applied if the optional includeExceptions argument is set to true.
The new signature should be:
function generateStyleSheetForDomain(domain, specificOnly = false, includeSelectors = false, includeExceptions = false)
The return value should always contain three properties: code, selectors, and exceptions. By default exceptions should be null, but if includeExceptions is true then it should be the list of exceptions (empty array if no exceptions apply).
Performance should not be negatively affected for the default case in which both includeSelectors and includeExceptions are false.
Integration Notes
Using the trace in the call to elemHide.generateStyleSheetsForDomain as an additoinal argument should suffice for getting the data to the UI. (will update after testing)
Hints for testers
The change here should not cause any change in behavior in the extension, unless the extension makes further changes (for which please see the relevant ticket for the Platform or UI modules (e.g. #6427)).
In order to ensure this, a simple test should suffice:
- Add two filters ##.foo and example.com##.bar and see that they hide the elements <div class="foo">Foo</div> and <div class="bar">Bar</div> on example.com.
- On any other domain, Foo should be hidden but Bar should still be visible.
- Add an exception foo.example.com#@#.foo and see that on foo.example.com only Foo is visible and Bar is hidden instead. Verify 1 and 2 again.
- Update the filter example.com##.bar to make it example.com,~bar.example.com##.bar and see that on bar.example.com Bar is visible but Foo is hidden. Verify 1, 2, and 3 again.
Attachments (1)
Change History (27)
comment:1 Changed on 05/07/2018 at 08:51:16 PM by jsonesen
- Cc jsonesen added
- Owner set to jsonesen
comment:2 Changed on 05/08/2018 at 10:16:45 AM by kzar
- Cc mjethani added
comment:3 follow-up: ↓ 5 Changed on 05/10/2018 at 02:55:00 AM by mjethani
Do we have to include all exceptions or only one that matched (and prevented the filter's selector from getting added)? If it's only one then it should be straightforward, just take the value returned by ElemHide.getException and add it to the return value.
The return value should then be {selectors, exceptions}.
I don't think this will affect performance much, and in any case this should only be done if the DevTools panel is open.
PS: By the way I would prefer if provideExceptionFilters were called withExceptions instead. It's shorter and conveys just as much. Also I think we only want to return the filter text, not the actual Filter object.
comment:4 Changed on 05/10/2018 at 03:02:56 AM by mjethani
So right now we have this code in ElemHide.getSelectorsForDomain:
if (!isIncluded) { excluded.add(filter); } else if ((excluded.size == 0 || !excluded.has(filter)) && !this.getException(filter, domain)) { selectors.push(filter.selector); }
We can just change it to:
if (!isIncluded) { excluded.add(filter); } else if (excluded.size == 0 || !excluded.has(filter)) { let exception = this.getException(filter, domain); if (!exception) selectors.push(filter.selector); else if (withExceptions) exceptions.push(exception.text); } ... if (withExceptions) return {selectors, exceptions}; return selectors;
And that should be enough.
Note that I'm trying to avoid calling ElemHide.getException for most domains in one of my patches so things might get interesting.
comment:5 in reply to: ↑ 3 Changed on 05/10/2018 at 08:08:08 AM by kzar
- Blocked By 6652 added
- Description modified (diff)
Since we're already working on some changes to this code I've marked this as being blocked by #6652.
Replying to mjethani:
Do we have to include all exceptions or only one that matched (and prevented the filter's selector from getting added)?
Yea, just the one that matched is fine.
PS: By the way I would prefer if provideExceptionFilters were called withExceptions instead.
Fine by me.
comment:6 Changed on 11/19/2018 at 02:00:07 PM by mjethani
- Cc tjansen added
comment:7 follow-up: ↓ 9 Changed on 11/19/2018 at 02:00:46 PM by mjethani
- Blocked By 6652 removed
I'll probably close #6652 since we have come a long way, but anyway it should not be blocking this issue now.
Changed on 11/19/2018 at 02:40:53 PM by mjethani
Apply on revision 107b12edc115
comment:8 Changed on 11/19/2018 at 02:43:49 PM by mjethani
Hi Timm, I have attached a patch that should apply on changeset 107b12edc115 in adblockpluscore. If you pass includeSelectors=trueto generateStyleSheetForDomain you'll get a array of selectors. If you additionally pass withExceptions=true, the array will contain an exceptions property, which will be another array containing the exceptions.
I hope this helps.
comment:10 Changed on 11/20/2018 at 02:13:01 AM by mjethani
Jon, if you look further into this, a few things:
- We have already optimized the style sheet generation for the default case (no DevTools panel open), I don't think we should touch that code path at all now
- If DevTools is open, we follow a slightly different path, because we need to return the list of selectors as well, which needs to be dynamically generated
- If DevTools needs the exceptions as well, we should have a completely different code path: In the generateStyleSheetForDomain function, we should check if the proposed withExceptions flag has been passed, and if this is the case then we should just do all of that in a separate block, and also use a different version of getConditionalSelectors (let's call it getConditionalSelectorsWithExceptions) that returns the exceptions as well similar to the patch I posted
These are some of my thoughts on this right now.
For context, DevTools is not open for most users most of the time, and it is also not there on mobile. We should not hurt the performance here for the special case of DevTools being open in the desktop extension.
comment:11 Changed on 03/05/2019 at 07:59:46 PM by jsonesen
- Cc sporz added
comment:12 Changed on 03/07/2019 at 03:16:42 PM by jsonesen
- Cc timm added
comment:13 Changed on 03/07/2019 at 03:26:13 PM by jsonesen
- Cc timm removed
comment:14 Changed on 03/12/2019 at 01:52:06 AM by jsonesen
- Description modified (diff)
comment:15 Changed on 03/12/2019 at 05:34:45 PM by jsonesen
comment:16 Changed on 03/16/2019 at 03:24:40 PM by mjethani
- Description modified (diff)
comment:17 Changed on 03/16/2019 at 03:30:00 PM by mjethani
- Description modified (diff)
comment:18 Changed on 03/17/2019 at 08:57:48 AM by mjethani
- Summary changed from Have getSelectorsForDomain provide exception filters if requested to Have generateStyleSheetForDomain provide exception filters if requested
comment:19 Changed on 04/08/2019 at 09:37:01 PM by abpbot
A commit referencing this issue has landed:
Issue 6428 - Expose elemHideExceptions for a domain on request
comment:20 Changed on 04/09/2019 at 01:40:29 AM by jsonesen
- Description modified (diff)
- Resolution set to fixed
- Status changed from reviewing to closed
comment:21 Changed on 04/09/2019 at 12:48:17 PM by mjethani
- Description modified (diff)
comment:22 Changed on 04/09/2019 at 12:49:57 PM by mjethani
- Description modified (diff)
comment:23 Changed on 04/09/2019 at 01:00:22 PM by mjethani
- Description modified (diff)
comment:24 Changed on 04/09/2019 at 07:13:07 PM by jsonesen
- Resolution fixed deleted
- Status changed from closed to reopened
comment:25 Changed on 04/09/2019 at 07:14:26 PM by mjethani
- Resolution set to fixed
- Status changed from reopened to closed
comment:26 Changed on 07/26/2019 at 03:12:34 AM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Working as described.
ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809
ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2
Note: Manish and I have recently made quite a few changes to this code and are still working on one.