Issue 29575739: Issue 5864 - Remove previous style sheet before adding one (Closed)
|
Created:
Oct. 13, 2017, 11:59 p.m. by Manish Jethani Modified:
Jan. 25, 2018, 4:15 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove style sheet if selectors list is empty #
Total comments: 1
Patch Set 3 : Rename inject to inline #Patch Set 4 : Maintain separate style sheet for emulated selectors #
Total comments: 6
Patch Set 5 : Always remove old rules #
Total comments: 5
Patch Set 6 : Remove some code duplication #Patch Set 7 : Create style sheet in include.preload.js #
Total comments: 9
Patch Set 8 : Maintain all style sheets in lib/cssInjection.js #
Total comments: 2
Patch Set 9 : Rebase #Patch Set 10 : Inline replaceStyleSheet #
Total comments: 16
Patch Set 11 : Rebase and fix errors #Patch Set 12 : Use "standard" group name #Patch Set 13 : Fix rebase #
MessagesTotal messages: 26
Patch Set 1
Currently we just keep adding new style sheets every time the DOM is updated and
something new is affected.
Take this HTML for example:
<style>
.greeting1 { color: blue }
</style>
<div>
<h1 class="greeting1">Hello</h1>
<h1 class="greeting2">Hi</h1>
</div>
<script>
setTimeout(() =>
{
let style = document.createElement("style");
style.innerHTML = ".greeting2 { color: blue }";
document.head.appendChild(style);
},
3000);
</script>
On Firefox, if you open the inspector and select the "greeting1" node, you'll
see that is it affected by two style sheets.
This patch will remove the previous style sheet before injecting a new one.
https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js
File include.preload.js (right):
https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js#...
include.preload.js:452: this.injectSelectors(selectors, filters);
We should also do the same for when styles are injected directly into the DOM.
For this we'll have to maintain two <style> elements, one for normal and one for
emulation, and injectSelectors would have to simply wipe the emulation style
sheet before adding any new selectors to it.
https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js#...
include.preload.js:458: selectors,
Note that earlier in this function we're exiting if the list of selectors is
empty. This means if there are any style sheets already injected, they should be
removed, but they won't. We should make sure that if the list of selectors is
empty then there are no style sheets left in the fame. We need to update the
logic here a bit.
https://codereview.adblockplus.org/29575739/diff/29575740/lib/cssInjection.js
File lib/cssInjection.js (right):
https://codereview.adblockplus.org/29575739/diff/29575740/lib/cssInjection.js...
lib/cssInjection.js:38: function addStyleSheet(tabId, frameId, selectors)
hideElements is now addStyleSheet
On 2017/10/14 00:08:03, Manish Jethani wrote: > Take this HTML for example: > [...] I forgot to add, you must add the filter "localhost#?#h1:-abp-properties(color: blue)" first (assuming you're loading the page off localhost).
It's a little worse actually. If you go into the console and enter "document.head.removeChild(document.head.children[0])" the Hello element will not get displayed again, even though no blue color applies to it any more.
Patch Set 2: Remove style sheet if selectors list is empty https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29575740/include.preload.js#... include.preload.js:458: selectors, On 2017/10/14 00:08:03, Manish Jethani wrote: > Note that earlier in this function we're exiting if the list of selectors is > empty. This means if there are any style sheets already injected, they should be > removed, but they won't. We should make sure that if the list of selectors is > empty then there are no style sheets left in the fame. We need to update the > logic here a bit. Done. https://codereview.adblockplus.org/29575739/diff/29576567/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576567/include.preload.js#... include.preload.js:470: this.addSelectorsInline(selectors); injectSelectors is now addSelectorsInline; the new injectSelectors injects a user style sheet.
Patch Set 3: Rename inject to inline Clarifying the terminology a bit. "inject" means inject a user style sheet, "inline" means add the selectors to an inline style sheet.
Patch Set 4: Maintain separate style sheet for emulated selectors This takes care of inline style sheets as well. https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:395: message.replaceSelectors = this.injectedSelectors; Renamed for clarity. https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:403: if (emulated && this.emulatedStyle) So there's a separate style sheet for emulated selectors and we remove it first before adding a new one in its place. https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:486: this.addSelectorsInline(selectors, filters, true); addSelectors is now only called for emulated selectors. For the normal selectors we call addSelectorsInline directly. https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:520: if (this.style && this.style.parentNode) This needs to be parentNode, not parentElement, because an element added to a shadow root has no parent element.
Patch Set 5: Always remove old rules https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:520: if (this.style && this.style.parentNode) On 2017/10/14 14:23:22, Manish Jethani wrote: > This needs to be parentNode, not parentElement, because an element added to a > shadow root has no parent element. Actually, why is this needed? The styles here will only be added once. This code is not going to run again, unless I'm missing something. Removed. https://codereview.adblockplus.org/29575739/diff/29576594/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576594/include.preload.js#... include.preload.js:345: this.styles = new Map(); Now it's a map. https://codereview.adblockplus.org/29575739/diff/29576594/include.preload.js#... include.preload.js:388: chrome.runtime.sendMessage({ Keeping it simple. https://codereview.adblockplus.org/29575739/diff/29576594/include.preload.js#... include.preload.js:404: style.sheet.deleteRule(0); Using CSSStyleSheet.deleteRule here. https://codereview.adblockplus.org/29575739/diff/29576594/include.preload.js#... include.preload.js:519: this.addSelectorsInline(response.selectors); Note that we don't have to pass the group name here, undefined is also a valid key in a map. https://codereview.adblockplus.org/29575739/diff/29576594/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29575739/diff/29576594/lib/cssInjection.js... lib/cssInjection.js:59: port.on("elemhide.getSelectors", (message, sender) => Just renaming for consistency.
Patch Set 6: Remove some code duplication
Patch Set 7: Create style sheet in include.preload.js It makes sense to create the style sheet in include.preload.js.
https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576591/include.preload.js#... include.preload.js:520: if (this.style && this.style.parentNode) On 2017/10/14 21:35:33, Manish Jethani wrote: > On 2017/10/14 14:23:22, Manish Jethani wrote: > > This needs to be parentNode, not parentElement, because an element added to a > > shadow root has no parent element. > > Actually, why is this needed? The styles here will only be added once. This code > is not going to run again, unless I'm missing something. OK, this code is called from composer.postload.js as well. But now these lines are in addSelectorsInline, where they belong, so that case should be covered as well.
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:490: this.style = null; What if the "Block element" dialog (from the browser action popup) is used? After adding the new filters, it calls ElemHide.apply() again, to re-apply all filters. It seems this never worked as intended with user stylesheets. But if you remove this code it won't work without user stylesheets anymore either. https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:386: return null; .sheet.insertRule() fails if we return null here.
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:490: this.style = null; On 2017/10/17 22:00:38, Sebastian Noack wrote: > What if the "Block element" dialog (from the browser action popup) is used? > After adding the new filters, it calls ElemHide.apply() again, to re-apply all > filters. It seems this never worked as intended with user stylesheets. But if > you remove this code it won't work without user stylesheets anymore either. No, now the removal of the previous style sheet is in addSelectorsInline Rather instead of removing the style element from the DOM we simply remove all the rules from the sheet using CSSStyleSheet.deleteRule https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:386: return null; On 2017/10/17 22:00:38, Sebastian Noack wrote: > .sheet.insertRule() fails if we return null here. Yes, but we have the following code earlier in addSelectorsInline: if (selectors.length == 0) return; So it never gets to that part if the list is empty.
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:490: this.style = null; On 2017/10/17 22:14:27, Manish Jethani wrote: > On 2017/10/17 22:00:38, Sebastian Noack wrote: > > What if the "Block element" dialog (from the browser action popup) is used? > > After adding the new filters, it calls ElemHide.apply() again, to re-apply all > > filters. It seems this never worked as intended with user stylesheets. But if > > you remove this code it won't work without user stylesheets anymore either. > > No, now the removal of the previous style sheet is in addSelectorsInline > > Rather instead of removing the style element from the DOM we simply remove all > the rules from the sheet using CSSStyleSheet.deleteRule Doesn't that cause ElemHideEmulation when adding selectors, to remove all selectors previously added for regular element hiding? https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:386: return null; On 2017/10/17 22:14:27, Manish Jethani wrote: > On 2017/10/17 22:00:38, Sebastian Noack wrote: > > .sheet.insertRule() fails if we return null here. > > Yes, but we have the following code earlier in addSelectorsInline: > > if (selectors.length == 0) > return; > > So it never gets to that part if the list is empty. So this check is redundant here. It seems in the other case where createRule() is called selectors wouldn't be empty either, and if so returning null would explode there as well, or do I miss something?
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:490: this.style = null; On 2017/10/18 00:53:07, Sebastian Noack wrote: > On 2017/10/17 22:14:27, Manish Jethani wrote: > > On 2017/10/17 22:00:38, Sebastian Noack wrote: > > > What if the "Block element" dialog (from the browser action popup) is used? > > > After adding the new filters, it calls ElemHide.apply() again, to re-apply > all > > > filters. It seems this never worked as intended with user stylesheets. But > if > > > you remove this code it won't work without user stylesheets anymore either. > > > > No, now the removal of the previous style sheet is in addSelectorsInline > > > > Rather instead of removing the style element from the DOM we simply remove all > > the rules from the sheet using CSSStyleSheet.deleteRule > > Doesn't that cause ElemHideEmulation when adding selectors, to remove all > selectors previously added for regular element hiding? ElemHideEmulation styles are in this.styles.get("emulation"), while normal styles are in this.styles.get(undefined). These are two different style "groups" now. https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:386: return null; On 2017/10/18 00:53:06, Sebastian Noack wrote: > On 2017/10/17 22:14:27, Manish Jethani wrote: > > On 2017/10/17 22:00:38, Sebastian Noack wrote: > > > .sheet.insertRule() fails if we return null here. > > > > Yes, but we have the following code earlier in addSelectorsInline: > > > > if (selectors.length == 0) > > return; > > > > So it never gets to that part if the list is empty. > > So this check is redundant here. It seems in the other case where createRule() > is called selectors wouldn't be empty either, and if so returning null would > explode there as well, or do I miss something? We still need that check for this line of code in addSelectors: this.injectStyleSheet(this.createRule(selectors)); If there are no selectors, injectStyleSheet will get null, which is also the initial value of this.injectStyleSheet. Basically it can be either null or a non-empty array.
https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29576600/include.preload.js#... include.preload.js:386: return null; On 2017/10/18 01:48:37, Manish Jethani wrote: > On 2017/10/18 00:53:06, Sebastian Noack wrote: > > On 2017/10/17 22:14:27, Manish Jethani wrote: > > > On 2017/10/17 22:00:38, Sebastian Noack wrote: > > > > .sheet.insertRule() fails if we return null here. > > > > > > Yes, but we have the following code earlier in addSelectorsInline: > > > > > > if (selectors.length == 0) > > > return; > > > > > > So it never gets to that part if the list is empty. > > > > So this check is redundant here. It seems in the other case where createRule() > > is called selectors wouldn't be empty either, and if so returning null would > > explode there as well, or do I miss something? > > We still need that check for this line of code in addSelectors: > > this.injectStyleSheet(this.createRule(selectors)); > > If there are no selectors, injectStyleSheet will get null, which is also the > initial value of this.injectStyleSheet. Basically it can be either null or a > non-empty array. I mean the initial value of "this.injectedStyleSheet"
Patch Set 8: Maintain all style sheets in lib/cssInjection.js We were not removing the old style sheet in the case of "Block element" Now all style sheets are maintained in the background page. https://codereview.adblockplus.org/29575739/diff/29582628/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582628/include.preload.js#... include.preload.js:440: let selector = preparedSelectors.slice( Reverted to original code. https://codereview.adblockplus.org/29575739/diff/29582628/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29575739/diff/29582628/lib/cssInjection.js... lib/cssInjection.js:122: port.on("elemhide.injectSelectors", (message, sender) => This is now again "injectSelectors"
Patch Set 9: Rebase
Patch Set 10: Inline replaceStyleSheet
Once again really sorry for being so slow to review! https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:499: else if (this.tracer) This logic seems different now, did you change it on purpose? https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:347: this.inline = true; How come you renamed this one? (I'm not against renaming it, just curious really.) https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:382: addSelectorsInline(selectors, filters, groupName) Just me or is the `filters` parameter not used? https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:505: this.addSelectorsInline(response.selectors); Shouldn't we pass the other parameters (well `groupName` anyway) here too? https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js... lib/cssInjection.js:46: chrome.tabs.removeCSS(tabId, { Should be `browser.tabs.removeCSS`? I guess we also need to check the API exists and add it to the webextension pollyfills? https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js... lib/cssInjection.js:54: function updateFrameStyles(pageId, frameId, selectors, groupName) I think `pageId` should be `tabId`, though you refer to both below.
Patch Set 11: Rebase and fix errors https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:499: else if (this.tracer) On 2018/01/24 14:30:53, kzar wrote: > This logic seems different now, did you change it on purpose? Yes, so there's a new function now called addSelectorsInline, and it doesn't do any tracing. We have to remove the else condition here so we trace for all cases. https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:347: this.inline = true; On 2018/01/24 14:30:53, kzar wrote: > How come you renamed this one? (I'm not against renaming it, just curious > really.) Yeah, this was for clarity [1] since we already use "inject" for tabs.insertCSS [1]: https://codereview.adblockplus.org/29575739/#msg6 https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:382: addSelectorsInline(selectors, filters, groupName) On 2018/01/24 14:30:53, kzar wrote: > Just me or is the `filters` parameter not used? True, removed. https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:505: this.addSelectorsInline(response.selectors); On 2018/01/24 14:30:53, kzar wrote: > Shouldn't we pass the other parameters (well `groupName` anyway) here too? Yeah so the default group is undefined. We could just use an explicit name here like "default" or something. https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js... lib/cssInjection.js:46: chrome.tabs.removeCSS(tabId, { On 2018/01/24 14:30:53, kzar wrote: > Should be `browser.tabs.removeCSS`? I guess we also need to check the API exists > and add it to the webextension pollyfills? Yes, sorry this patch is really old. Done. (We don't have to check for this API since the code will never get there if userStyleSheetsSupported is false.) https://codereview.adblockplus.org/29575739/diff/29582637/lib/cssInjection.js... lib/cssInjection.js:54: function updateFrameStyles(pageId, frameId, selectors, groupName) On 2018/01/24 14:30:53, kzar wrote: > I think `pageId` should be `tabId`, though you refer to both below. Fixed.
Otherwise this LGTM https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (left): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:499: else if (this.tracer) On 2018/01/24 16:37:44, Manish Jethani wrote: > On 2018/01/24 14:30:53, kzar wrote: > > This logic seems different now, did you change it on purpose? > > Yes, so there's a new function now called addSelectorsInline, and it doesn't do > any tracing. We have to remove the else condition here so we trace for all > cases. Acknowledged. https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:347: this.inline = true; On 2018/01/24 16:37:44, Manish Jethani wrote: > On 2018/01/24 14:30:53, kzar wrote: > > How come you renamed this one? (I'm not against renaming it, just curious > > really.) > > Yeah, this was for clarity [1] since we already use "inject" for tabs.insertCSS > > [1]: https://codereview.adblockplus.org/29575739/#msg6 Acknowledged. https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:505: this.addSelectorsInline(response.selectors); On 2018/01/24 16:37:44, Manish Jethani wrote: > On 2018/01/24 14:30:53, kzar wrote: > > Shouldn't we pass the other parameters (well `groupName` anyway) here too? > > Yeah so the default group is undefined. We could just use an explicit name here > like "default" or something. Yea, I think making the name here explicit would make the code a little more obvious.
Patch Set 12: Use "standard" group name https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29575739/diff/29582637/include.preload.js#... include.preload.js:505: this.addSelectorsInline(response.selectors); On 2018/01/25 11:52:37, kzar wrote: > On 2018/01/24 16:37:44, Manish Jethani wrote: > > On 2018/01/24 14:30:53, kzar wrote: > > > Shouldn't we pass the other parameters (well `groupName` anyway) here too? > > > > Yeah so the default group is undefined. We could just use an explicit name > here > > like "default" or something. > > Yea, I think making the name here explicit would make the code a little more > obvious. OK, I'm using the name "standard" here since these are standard CSS selectors, we just use them as they are in the filters.
LGTM, Sebastian you want to take another look?
Patch Set 13: Fix rebase Sorry the previous rebase wasn't right, now fixed.
On 2018/01/25 14:34:45, kzar wrote: > LGTM, Sebastian you want to take another look? I wouldn't get around to have a deep look soon. Feel free to remove me as a reviewer from this one, and push it (into the "next" bookmark). 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
