include.preload.js - Issue 5989914281771008: Use Shadow DOM for element hiding

Side by Side Diff: include.preload.js

Issue 5989914281771008: Use Shadow DOM for element hiding (Closed)

Patch Set: Created April 15, 2014, 9:48 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
1 /*	1 /*

2 * This file is part of Adblock Plus <http://adblockplus.org/>,	2 * This file is part of Adblock Plus <http://adblockplus.org/>,

3 * Copyright (C) 2006-2014 Eyeo GmbH	3 * Copyright (C) 2006-2014 Eyeo GmbH

4 *	4 *

5 * Adblock Plus is free software: you can redistribute it and/or modify	5 * Adblock Plus is free software: you can redistribute it and/or modify

6 * it under the terms of the GNU General Public License version 3 as	6 * it under the terms of the GNU General Public License version 3 as

7 * published by the Free Software Foundation.	7 * published by the Free Software Foundation.

8 *	8 *

9 * Adblock Plus is distributed in the hope that it will be useful,	9 * Adblock Plus is distributed in the hope that it will be useful,

10 * but WITHOUT ANY WARRANTY; without even the implied warranty of	10 * but WITHOUT ANY WARRANTY; without even the implied warranty of

11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

12 * GNU General Public License for more details.	12 * GNU General Public License for more details.

13 *	13 *

14 * You should have received a copy of the GNU General Public License	14 * You should have received a copy of the GNU General Public License

15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

16 */	16 */

17	17

18 var SELECTOR_GROUP_SIZE = 20;	18 var SELECTOR_GROUP_SIZE = 20;

19	19

20 var elemhideElt = null;	20 var elemhideElt = null;

21	21

22 // Sets the currently used CSS rules for elemhide filters	22 // Sets the currently used CSS rules for elemhide filters

23 function setElemhideCSSRules(selectors)	23 function setElemhideCSSRules(selectors)

24 {	24 {

25 if (elemhideElt && elemhideElt.parentNode)	25 if (elemhideElt && elemhideElt.parentNode)

26 elemhideElt.parentNode.removeChild(elemhideElt);	26 elemhideElt.parentNode.removeChild(elemhideElt);
Sebastian Noack 2014/04/15 09:58:06 Storing the inserted <style> tag into a global var Storing the inserted <style> tag into a global variable in order to remove it everytime setElemhideCSSRules() is called seems unneeded to me, since it is only called once or did I missed something? Wladimir Palant 2014/04/15 13:13:14 I somewhat remember that AdThwart had issues with Show quoted text On 2014/04/15 09:58:06, Sebastian Noack wrote: > Storing the inserted <style> tag into a global variable in order to remove it > everytime setElemhideCSSRules() is called seems unneeded to me, since it is only > called once or did I missed something? I somewhat remember that AdThwart had issues with content scripts being applied multiple times for some reason. I am fairly certain that this code is safe to remove now.
27	27

28 if (!selectors)	28 if (selectors.length == 0)
	Sebastian Noack 2014/04/15 09:58:06 Skip inserting a style tag, if there are no elemen Skip inserting a style tag, if there are no element hiding rules for the current website. Previously we only bailed out when the website itself is whitelisted. Wladimir Palant 2014/04/15 13:13:14 That doesn't make much difference in reality - if Show quoted text On 2014/04/15 09:58:06, Sebastian Noack wrote: > Skip inserting a style tag, if there are no element hiding rules for the current > website. Previously we only bailed out when the website itself is whitelisted. That doesn't make much difference in reality - if the user has a filter list and didn't whitelist the website then some element hiding rules will most certainly apply to every domain. Still nothing wrong with the change of course. Sebastian Noack 2014/04/15 19:21:54 I am aware that at least when using EASYList there Show quoted text On 2014/04/15 13:13:14, Wladimir Palant wrote: > On 2014/04/15 09:58:06, Sebastian Noack wrote: > > Skip inserting a style tag, if there are no element hiding rules for the > current > > website. Previously we only bailed out when the website itself is whitelisted. > > That doesn't make much difference in reality - if the user has a filter list and > didn't whitelist the website then some element hiding rules will most certainly > apply to every domain. Still nothing wrong with the change of course. I am aware that at least when using EASYList there are always some element hiding filters that are included on all websites. However handling that case doesn't add any complexity, and IMHO it makes the API even more simple and robust against wrong assumptions when always an array is returned.
29 return;	29 return;

30	30

31 elemhideElt = document.createElement("style");

32 elemhideElt.setAttribute("type", "text/css");

33

34 // Try to insert the style into the <head> tag, inserting directly under the	31 // Try to insert the style into the <head> tag, inserting directly under the

35 // document root breaks dev tools functionality:	32 // document root breaks dev tools functionality:

36 // http://code.google.com/p/chromium/issues/detail?id=178109	33 // http://code.google.com/p/chromium/issues/detail?id=178109

37 (document.head \|\| document.documentElement).appendChild(elemhideElt);	34 var container = document.head \|\| document.documentElement;

	35

	36 // Use Shadow DOM if available to don't mess with web pages

	37 // that rely on the order of their own <style> tags (#309)

	38 if ("createShadowRoot" in container)

	39 container = container.createShadowRoot();
	Wladimir Palant 2014/04/15 13:13:14 Given that this standard is still very much in flu Given that this standard is still very much in flux we should not expect our code to work with the final version of Web Components. It should be better to check for webkitCreateShadowRoot only, at least for now. Sebastian Noack 2014/04/15 19:21:54 Done. Show quoted text On 2014/04/15 13:13:14, Wladimir Palant wrote: > Given that this standard is still very much in flux we should not expect our > code to work with the final version of Web Components. It should be better to > check for webkitCreateShadowRoot only, at least for now. Done.
	40 else if ("webkitCreateShadowRoot" in container)

	41 container = container.webkitCreateShadowRoot();
	Wladimir Palant 2014/04/15 13:13:14 Please add a <shadow> element to our shadow root t Please add a <shadow> element to our shadow root to make sure any content defined by the document or other shadow roots is still in the DOM tree. Interestingly, Chrome doesn't seem to implement HTMLShadowElement.olderShadowRoot yet. This means that by adding one more shadow root with only the <shadow> element we can protect our style element from website accesses. Not sure whether this is worth doing. Sebastian Noack 2014/04/15 19:21:54 Done. Show quoted text On 2014/04/15 13:13:14, Wladimir Palant wrote: > Please add a <shadow> element to our shadow root to make sure any content > defined by the document or other shadow roots is still in the DOM tree. > > Interestingly, Chrome doesn't seem to implement > HTMLShadowElement.olderShadowRoot yet. This means that by adding one more shadow > root with only the <shadow> element we can protect our style element from > website accesses. Not sure whether this is worth doing. Done.
	42

	43 elemhideElt = document.createElement("style");

	44 elemhideElt.setAttribute("type", "text/css");

	45 container.appendChild(elemhideElt);
	Wladimir Palant 2014/04/15 13:13:14 Will these styles affect the regular elements or o Will these styles affect the regular elements or only the shadow DOM? From http://www.w3.org/TR/shadow-dom/#styles it seems that the latter is the case. That was actually the reason why we dismissed using Shadow DOM for element hiding earlier. It seems that in order to have the styles apply to the document we have to create the shadow root on the document element and then use the ::-webkit-distributed() pseudo-element for each selector. I wonder whether we can measure the overhead (CPU time and memory) caused by this. Sebastian Noack 2014/04/15 19:21:54 You are right, it actually didn't worked with the Show quoted text On 2014/04/15 13:13:14, Wladimir Palant wrote: > Will these styles affect the regular elements or only the shadow DOM? From > http://www.w3.org/TR/shadow-dom/#styles it seems that the latter is the case. > That was actually the reason why we dismissed using Shadow DOM for element > hiding earlier. > > It seems that in order to have the styles apply to the document we have to > create the shadow root on the document element and then use the > ::-webkit-distributed() pseudo-element for each selector. I wonder whether we > can measure the overhead (CPU time and memory) caused by this. You are right, it actually didn't worked with the previous patch. I confused element hiding with element collapsing when I tested it. But after using the document element as shadow root, adding a <shadow> node to it and wrapping the selectors with ::-webkit-distributed() it works perfectly.
38	46

39 var elt = elemhideElt; // Use a local variable to avoid racing conditions	47 var elt = elemhideElt; // Use a local variable to avoid racing conditions

40 function setRules()	48 function setRules()

41 {	49 {

42 if (!elt.sheet)	50 if (!elt.sheet)
	Sebastian Noack 2014/04/15 19:21:54 I also got rid of that code. The sheet property is I also got rid of that code. The sheet property is present as soon as you insert the element into the DOM. It works even with Safari 5.1, which is the oldest webkit browser I could find for download. Do you remember why we had that code in the first place? Wladimir Palant 2014/04/15 21:19:47 I didn't remember but hg blame points to https://a I didn't remember but hg blame points to https://adblockplus.org/forum/viewtopic.php?t=8834. As with any issues that cannot be reproduced, it is really hard to tell whether the issue is gone. However, it seems that this was running on a timeout even before the change in question - so older Chrome versions weren't initializing the element.sheet property immediately. Please verify that your code works in Chrome 19. You can get Chromium 19 (trunk build, right before the release branch was created) from https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.htm... (builds for other operating systems are available as well).
43 {	51 {

44 // Stylesheet didn't initialize yet, wait a little longer	52 // Stylesheet didn't initialize yet, wait a little longer

45 window.setTimeout(setRules, 0);	53 window.setTimeout(setRules, 0);

46 return;	54 return;

47 }	55 }

48	56

49 // WebKit apparently chokes when the selector list in a CSS rule is huge.	57 // WebKit apparently chokes when the selector list in a CSS rule is huge.

50 // So we split the elemhide selectors into groups.	58 // So we split the elemhide selectors into groups.

51 for (var i = 0, j = 0; i < selectors.length; i += SELECTOR_GROUP_SIZE, j++)	59 for (var i = 0, j = 0; i < selectors.length; i += SELECTOR_GROUP_SIZE, j++)

52 {	60 {

(...skipping 61 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
114 ext.backgroundPage.sendMessage({type: "add-key-exception", token: attr});	122 ext.backgroundPage.sendMessage({type: "add-key-exception", token: attr});

115	123

116 ext.backgroundPage.sendMessage({type: "get-selectors"}, setElemhideCSSRules);	124 ext.backgroundPage.sendMessage({type: "get-selectors"}, setElemhideCSSRules);

117 }	125 }

118	126

119 // In Chrome 18 the document might not be initialized yet	127 // In Chrome 18 the document might not be initialized yet

120 if (document.documentElement)	128 if (document.documentElement)

121 init();	129 init();

122 else	130 else

123 window.setTimeout(init, 0);	131 window.setTimeout(init, 0);

OLD	NEW

« no previous file with comments | « background.js ('k') | no next file » | no next file with comments »