New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 632009 link

Starred by 49 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug

Blocking:
issue 671907


Show other hotlists

Hotlists containing this issue:
style-dev-current
Hotlist-1


Sign in to add a comment

Styles injected by extensions don't take precedence over the page's styles

Reported by k...@kzar.co.uk, Jul 27 2016

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36

Steps to reproduce the problem:
1. Install Adblock Plus
2. Remove all filter subscriptions
3. Add this filter: static.kzar.co.uk##.hideme
4. Browse to this page https://static.kzar.co.uk/chrome-style-problem/

What is the expected behavior?
The bold "HIDE ME" text is hidden.

What went wrong?
The text is not hidden.

WebStore page: https://chrome.google.com/webstore/detail/adblock-plus/cfhdojbkjhnklbpkdaibdccddilifddb?hl=sv

Did this work before? No 

Chrome version: 52.0.2743.82  Channel: stable
OS Version: Debian Stretch
Flash Version: Disabled

Adblock Plus (and other extensions) need a way to inject styles into a page that take precedence over the page's styles. Our only alternative is to use a content script to manually ensure all matching elements are hidden, but we would rather not do this as it would likely be pretty slow.

Note from implementor nainar@:
Here is the design document to implement this: https://docs.google.com/document/d/1e9qAYq1tc1mqHStIGIaafPN9_qiDFjBaG71UiO49PPA/edit#heading=h.8xel0fq1hzqn
 

Comment 1 by k...@kzar.co.uk, Jul 27 2016

Related issue in our tracker: https://issues.adblockplus.org/ticket/4171
Therefore I'd like to add a bit of information to show the impact this currently has on websites. The content script Adblock Plus injects uses the following features to work around the lack of proper user styles:

- Message passing: The background page passes all selectors (more than 10,000) to the content script which then transforms them into CSS rules that it injects into the page.

- Shadow DOM: It creates a Shadow Root on the document element to avoid interference by the website. Due to that it also needs to go through all of its CSS selectors (more than 10,000) and prepend "::content" to apply its styles to the entire document from within the Shadow DOM.

- MutationObserver: It uses a MutationObserver to observe changes to the subtree its styles are in (e.g. its Shadow Root or even the document element).

- Object.defineProperty(): It overwrites certain browser APIs (e.g. CSSStyleSheet.deleteRule) to protect its stylesheet from being removed.

Apart from the performance and memory benefits of removing some of those parts and moving others into the extension's background page (see also https://bugs.chromium.org/p/chromium/issues/detail?id=427527), there'd also be the added benefit of making any attempts by the website to identify the extension less reliable.

According to Chromium's Ojan (see https://bugs.chromium.org/p/chromium/issues/detail?id=347016#c3) the issue with user stylesheets was (a) the way it was implemented in WebKit and (b) that only few people were making use of them. So since Chromium has moved on to Blink and the fact that this feature would be used by the top two Chrome extensions (AdBlock and Adblock Plus) it might be worth revisiting https://bugs.chromium.org/p/chromium/issues/detail?id=427527.

Comment 3 Deleted

I don't see any connection between browser extensions and the regression of Firefox user numbers. In fact, a powerful extension API, and the rich extensions ecosystem, it lead to, with Adblock Plus as the most popular extension, seems to have been a driving cause of Firefox's success.

However, Firefox naturally lost users to Chrome, since Chrome entered the browser market which is no surprise. The increase of mobile users, with Chrome being the default browser on Android, certainly helped to accelerate that process.

As for this particular issue, while it also will help ad blocking, it is about all extensions that aim to improve/change the user experience on any website. And as browser extensions exist in the first place to empower the user to modify their user experience, there is not much of a point to give precedence to websites over extensions.

In fact, chrome.tabs.insertCSS() was used to insert a user stylesheet, which has precedence over the author stylesheets on the web page. However, at some point, support for user sylesheets was removed from WebKit (which Chrome was using back then), because it seemed that nobody was using user stylesheets, however, they seem to have ignored (or underestimated) the implication for extensions. So, FWIW, this is a regression.
Components: Blink>CSS Blink>DOM>ShadowDOM
This might block the deprecation or removal of Shadow DOM v0. See https://groups.google.com/a/chromium.org/d/msg/blink-dev/txIN7qDRFpU/g0SijZiwBgAJ
Cc: meade@chromium.org hayato@chromium.org
Status: Available (was: Unconfirmed)
Thank you for reaching us. We might fix this issue by changing the insertCSS API to be user stylesheets, which could take precedence over the page's styles.

cc-ing meade@


Comment 8 by meade@chromium.org, Dec 8 2016

I've removed comment 3 as it does not comply with our code of conduct. Please read the code of conduct here:
https://chromium.googlesource.com/chromium/src/+/master/CODE_OF_CONDUCT.md

Comment 9 by meade@chromium.org, Dec 8 2016

Hi Adblock Plus folks,

I've added this issue to my list of things to consider at 2017 planning. No promises yet, but we are looking into it!
Labels: -Type-Bug Type-Feature

Comment 11 by meade@chromium.org, Jan 19 2017

Labels: Objective
Owner: nainar@chromium.org
Status: Assigned (was: Available)
Hi everyone,

We're going to start looking into this from about February. Huzzah! I think it will look something like this (nainar to write design doc...)

- Reimplement user stylesheet level priority, which is lower than author rules, but higher for !important rules (user stylesheet always loses for regular rules, but always wins for !important rules)
- Implement them so that they are global (like the UserAgent stylesheet). This means that they will apply to shadow DOM (but descendant selectors etc wouldn't cross shadow boundaries).
Hello all!

I am quite excited about seeing your interest in implementing this! Recently I implemented an API extension to insertCSS/removeCSS in Firefox, for their WebExtension support, especially also for Adblock Plus.

This is https://bugzilla.mozilla.org/show_bug.cgi?id=1310026, maybe we can align these two APIs?

Best,
Tom

Comment 13 by meade@chromium.org, Jan 24 2017

Cc: ojan@chromium.org
Hi evilpies!

Fantastic! I took a brief look at your bugzilla bug, and it looks like what you've got is very similar to our plan. The only major difference, is AFAIK so far we were just planning on having all insertCSS behave at user stylesheet priority, instead of having the extension author specify it. Our reasoning is that we think all the users of insertCSS would rather have the user priority behaviour anyway.

What was your thinking around adding the CSSOrigin option in your extension schema? Do you think most users would prefer one way or the other?

Adding ojan, who has been helping us with our design.

Thanks!
Eddy
Great. I have to admit, I don't think we even considered changing the default behavior here. Just to maintain compatibility probably. If you think you can change the default behavior here, we could probably do this as well.

Comment 15 by ojan@chromium.org, Jan 26 2017

Cc: devlin@chromium.org
I'm assuming that basically everyone using the current API would actually prevent user-level stylesheets, so am hoping the level of breakage will be extremely minimal.

Mainly, I'd prefer for our CSS implementation not to do to support injecting multiple levels of stylesheet if we can avoid it.
I prefer user stylesheet by default.

I have found the current extensions api documentation says:

https://developer.chrome.com/extensions/api_index

> Optional. The list of CSS files to be injected into matching pages. These are injected in the order they appear in this array, before any DOM is constructed or displayed for the page.

Hmm. It sounds that this doc assumes that CSS files are injected as *author* stylesheets. We might want to update this documentation as well, if we are to use user stylesheet by default. Can we do that well?
Updating the docs to specify whether these are user or author style sheets sgtm.  Extensions have two mechanisms that allow this, manifest-specified css (https://developer.chrome.com/extensions/content_scripts) and chrome.tabs.insertCSS (https://developer.chrome.com/extensions/tabs#method-insertCSS).

At an implementation level, both of these are handled in ScriptInjection::InjectCss [1], which just uses WebDocument::insertStyleSheet().  If we can change insertStyleSheet() to either accept or default to user style sheets, it should be pretty straight forward.

I'd also agree with ojan@ that breakage would hopefully be small.
I'll also announce this on blink-dev and chromium-dev as a PSA. Does it sound like DevRel should be involved?
Labels: Update-Monthly
Description: Show this description
Hi everyone, 

Just to clarify that the Style team will start implementation work on this next couple of quarters when our plate clears up a bit. 

Thank you for the feedback!
I was asked on an adblockplus ticket (https://issues.adblockplus.org/ticket/4945) whether the solution for this is going to apply for shadow dom. I don't seem to have access to the ticket to reply, so I'll reply here: Yes, that is our intention. Rules specified at user stylesheets priority will apply to shadow dom, but descendant selectors and similar won't cross the boundaries.
Thanks for the reply Meade. I've removed you from that ticket again sorry for the spam there.
Blocking: 489954

Comment 25 by shans@chromium.org, Mar 23 2017

Blocking: -489954
Cc: nainar@chromium.org
Owner: ----
Status: Available (was: Assigned)
In reply to #15, while I agree and don't see any reason why any extension should expect their stylesheets being applied with any less precedence than possible, a way for extensions to detect whether insertCSS() is using user stylesheets would be very helpful.

Since we cannot immediately drop support for all previous Chromium versions which didn't support user stylesheets with insertCSS(), we have to fallback to content scripts for the transition period. But if this change is completely transparent to the API, we'd have to rely on the user agent string, which is not a reliable indicator.

On Firefox, we are now going catch the error insertCSS() is throwing, when the cssOrigin option is unknown, in order to detect user stylesheet support. If Chromium however will just imply user stylesheets without adding a new option, it would be great if there would be a property (e.g. chrome.tabs.INSERT_CSS_USER_STYLESHEETS) or something that we can rely on instead in order to detect that behavior.
In reply to Comment #27. 

We can definitely factor this concern into the design process when we move to implement it. Thank you for raising that concern. 

Comment 29 by ojan@chromium.org, Apr 7 2017

Why isn't the UserAgent string a reliable indicator? I agree that feature detection is generally preferred, but it's not clear to me why it's needed for this specific case.
> Why isn't the UserAgent string a reliable indicator?

For example, because there is a number of Chromium-based browsers and these have their own rules for the user agent. Also, user agent parsing is inherently error-prone. This could be solved in a generic way as well of course, e.g. by having runtime.getPlatformInfo() return browsing engine and its version.
Also mind that the user agent can bet set to any possible string though the --user-agent command line switch. We already got bug reports in the past, from users doing this, when we relied on the Chrome version from the user agent string for other stuff.
Components: -Blink>DOM>ShadowDOM

Comment 33 by kochi@chromium.org, Aug 31 2017

Style team, any updates on this?

Comment 35 by kochi@chromium.org, Aug 31 2017

Thanks for the update!

Comment 36 by kochi@chromium.org, Aug 31 2017

Blocking: 671907
Project Member

Comment 37 by bugdroid1@chromium.org, Oct 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30168b99604c48a32828f26b5acb2674beea1a5d

commit 30168b99604c48a32828f26b5acb2674beea1a5d
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Mon Oct 16 17:47:34 2017

Implement user style sheets

This is based on the design document at https://goo.gl/jzD8Zk

Browser extension developers expect their style sheets to be treated as
user style sheets, as "!important" user declarations have a higher
precedence than "!important" author declarations.

Therefore, in order to implement this behavior, this patch makes the
following changes:

 1.  MatchResult now maintains user rules in addition to UA and author
     rules
 2.  DocumentStyleSheetCollection no longer cares about injected style
     sheets
 3.  StyleEngine now treats injected style sheets as user style sheets
 4.  StyleResolver now calls its new MatchUserRules method right after
     calling MatchUARules
 5.  The ApplyMatchedStandardProperties method of StyleResolver applies
     important properties in the correct order

BUG= 632009 

Change-Id: If752b1af762f233dae49bb5bffd6d5e6a4b54acd
Reviewed-on: https://chromium-review.googlesource.com/641294
Commit-Queue: Rune Lillesveen <rune@opera.com>
Reviewed-by: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#509096}
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/AUTHORS
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/LayoutTests/http/tests/devtools/elements/styles-2/inject-stylesheet-expected.txt
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/LayoutTests/http/tests/devtools/elements/styles-2/inject-stylesheet.html
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/DocumentStyleSheetCollection.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/ElementRuleCollector.h
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/resolver/MatchResult.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/resolver/MatchResult.h
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/resolver/MatchResultTest.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/css/resolver/StyleResolver.h
[modify] https://crrev.com/30168b99604c48a32828f26b5acb2674beea1a5d/third_party/WebKit/Source/core/exported/WebDocument.cpp

Is Ojan's research on the backwards compatibility issues publicly available anywhere?

I've never seen an extension only use `!important` rules, and it astounds me that any developer would expect a lower precedence there. It's shocking to have an `#id` selector outmatched by a `span` reset, because overriding author styles is the exact reason extensions inject styles to begin with.

If anything, I'd expect extensions to have -more- importance than author styles (such as the Override origin) -- even though this would still be suboptimal as it'd prevent reuse of author rules in certain cases.

Is there any possibility of following Firefox here and allowing extensions to define the stylesheet origin?
Project Member

Comment 39 by bugdroid1@chromium.org, Oct 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed

commit bed9cb9d56adac6f50313b0e620a1aa2f0d871ed
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Sat Oct 28 23:02:07 2017

Support @keyframes rules in user style sheets

Previously @keyframes rules in style sheets injected by extensions were
managed in ScopedStyleResolver along with other author rules. With
crrev.com/c/641294 we started treating these injected style sheets as
user style sheets and moved the management of such rules to StyleEngine.
This patch fixes a regression caused by this change whereby
StyleResolver is no longer able to find @keyframes rules from these
style sheets.

StyleEngine now maintains a map of animation names to their
corresponding @keyframes rules found in user style sheets. If
StyleResolver is unable to find a given @keyframes rule among its author
rules, it looks it up in StyleEngine next.

BUG= 632009 , 776661 

Change-Id: I90462a0721cc75da14ee041c1a70736e4df99fb8
Reviewed-on: https://chromium-review.googlesource.com/735263
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512411}
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp

Project Member

Comment 40 by bugdroid1@chromium.org, Nov 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3857f5dccf0f16288947c8c3c79b2a9a5bbd72b5

commit 3857f5dccf0f16288947c8c3c79b2a9a5bbd72b5
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Wed Nov 01 16:46:46 2017

Support @font-face rules in user style sheets

@font-face rules in author style sheets are passed back to StyleEngine
via ScopedStyleResolver. With crrev.com/c/641294 we started treating
style sheets injected by extensions as user style sheets instead. Since
user style sheets apply to all scopes, they never go via
ScopedStyleResolver but are rather managed by StyleEngine directly.

crrev.com/c/641294 broke extensions using custom fonts because
StyleEngine fails to account for @font-face rules in user style sheets.

Fonts from both user and author style sheets must be maintained in the
same font cache. Even though style sheets can be added in any order,
@font-face rules in author style sheets must appear after those in user
style sheets. In order to achieve this result efficiently, we use a
"dirty" flag and refresh the font cache only once per cycle.

StyleEngine::ApplyRuleSetChanges may be called twice, once for user
style sheets (all scopes) and once for author style sheets (document
scope). If fonts have changed in user style sheets, we simply set the
dirty flag. If fonts have changed in author style sheets, we set the
dirty flag but also then refresh the font cache by first adding all the
@font-face rules from the active user style sheets and then re-adding
all the new author style sheets. This ensures that the font cache is
refreshed only once while the fonts are added in the correct order.

BUG= 632009 , 779048 

Change-Id: I58c1070af3ecae925e4afb91cbbb7cb00ee187fd
Reviewed-on: https://chromium-review.googlesource.com/743642
Commit-Queue: nainar <nainar@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513154}
[modify] https://crrev.com/3857f5dccf0f16288947c8c3c79b2a9a5bbd72b5/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/3857f5dccf0f16288947c8c3c79b2a9a5bbd72b5/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/3857f5dccf0f16288947c8c3c79b2a9a5bbd72b5/third_party/WebKit/Source/core/css/StyleEngineTest.cpp

So, this bug and the resulting design changes are actually causing problems in our extension, Reddit Enhancement Suite, that I think speak to a diametrically opposed philosophy here...

I fully understand why the AdBlock Plus devs have this request - but I'd like to give the other side of this and expose the negative effects this will have on other extension types...

Reddit Enhancement Suite, like lots of extensions, has the ability to add and change styles on a specific website (lots of extensions add the ability to add/change stylesheets on many sites, though)...

We've been overriding styles on reddit without issue for years by simply understanding what selectors are being used (because we can look at the site and the CSS) and using greater specificity.

I understand that for AdBlock's use case example above, that's not super feasible - because they'd need for either the end user or their extension to figure out / understand the specificity and then create a CSS rule that "wins" over it.

However, the flip side of this is that with these changes (if I understand correctly), current extensions that style other sites will need to change in one of two ways:

1) Start using !important everywhere in their CSS, which is pretty gross, and also causes problems for website authors who are trying to cooperatively style "with" or "for" such extensions - because now the extension is using !important and the site owner (or in reddit's case, a subreddit moderator) cannot really play "nicely" with the extension's stylesheet due to that fact.  (RES actually encourages people to style the elements it adds to the page, etc - and tons of people do that!)

2) We'll need to start injecting our styles using a <style> tag instead of using this API - which is less performant and more likely to result in a Flash of Unstyled Content -- which is minor in some cases, but rather annoying in others (e.g. a "night" or "dark" mode extension -- a white flash before the dark mode takes effect essentially negates the entire purpose)...

I fully understand AdBlock's reasoning for making this request, but I'm a little concerned that the resulting changes are going to result in larger consequences than have been discussed in this conversation as it stands thus far.

I'm also open to the ideas that:

1) I'm misinterpreting something here

2) there are alternative solutions that I'm not thinking of!

Thanks for reading!
To me it seems the obvious solution would be adding a cssOrigin option to tabs.insertCSS(), that lets you specify whether an author or user stylesheet should be injected, just like Firefox.
#42: And a similarly named field for the manifest-based css injections.

Let's also not forget about the DOM Level 2 "override" origin. It should come in handy for a number of use cases.
Cc: m.jeth...@eyeo.com
Owner: nainar@chromium.org
Status: Assigned (was: Available)
Marking this as Owned. To reflect the true status of the work here. 
Pinging Ojan also look at comment #38
Cc: dgozman@chromium.org
 Issue 776891  has been merged into this issue.
Labels: -Pri-2 -Type-Feature Pri-1 Type-Bug-Regression
Please see discussion in  issue 776891 . The change from #c37 regressed some existing user scenarios, so perhaps it makes sense to revert for now and re-evaluate?
Anyway, raising the priority of this to address the regression sooner.
I think issue 776878 is related to this also, though maybe the commit from comment 40 fixes it? It broke at the same time.
Project Member

Comment 48 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56daa9455a7ab0cc7af996980b741aa1ec82a42b

commit 56daa9455a7ab0cc7af996980b741aa1ec82a42b
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Fri Nov 10 06:24:46 2017

Apply custom properties in user style sheets

With crrev.com/c/641294 we introduced user style sheets, but without taking
into account custom properties. This patch adds support for custom properties
in user style sheets by calling StyleResolver::ApplyMatchedProperties for user
rules in the correct order.

BUG= 632009 , 781223 

Change-Id: I1c103fc37484835008b1b55e9b3b8099af50a48a
Reviewed-on: https://chromium-review.googlesource.com/754247
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515478}
[modify] https://crrev.com/56daa9455a7ab0cc7af996980b741aa1ec82a42b/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/56daa9455a7ab0cc7af996980b741aa1ec82a42b/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp

Project Member

Comment 49 by bugdroid1@chromium.org, Nov 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c26a7505fd2da86795521ed2a300dacbfb78853c

commit c26a7505fd2da86795521ed2a300dacbfb78853c
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Thu Nov 23 13:25:54 2017

Add origin option for injected style sheets

crrev.com/c/641294 replaced injected author style sheets with user style
sheets. There are, however, legitimate use cases for the former. For
one, specificity does not work across origin boundaries; in such cases
extension developers may be forced to use "!important" as a hack.

This patch reintroduces the author origin for injected style sheets and
makes it the default by bringing back the relevant code deleted in
crrev.com/c/641294

WebDocument::InsertStyleSheet now takes an optional second argument of
type WebDocument::CSSOrigin

BUG= 632009 

Change-Id: Ia6e7018411faf47411aabfb82e39158650ccedb4
Reviewed-on: https://chromium-review.googlesource.com/765642
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518913}
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/LayoutTests/http/tests/devtools/elements/styles-2/inject-stylesheet-expected.txt
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/LayoutTests/http/tests/devtools/elements/styles-2/inject-stylesheet.js
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/Source/core/css/DocumentStyleSheetCollection.cpp
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/Source/core/exported/WebDocument.cpp
[modify] https://crrev.com/c26a7505fd2da86795521ed2a300dacbfb78853c/third_party/WebKit/public/web/WebDocument.h

The origin option defaults to author. This should address the regressions. 
Labels: -Type-Bug-Regression Type-Bug
Cc: -m.jeth...@eyeo.com
Owner: m.jeth...@eyeo.com
Labels: -Update-Monthly

Comment 54 by kochi@chromium.org, Dec 25 2017

Status checking: as the original proposed change was reverted, AdBlock
is still blocked on migrating out of Shadow DOM V0? Am I understanding
the situation correct?
Does any other solution (than depending on user stylesheet's cascading
order) exist?

Comment 55 by woxxom@gmail.com, Dec 25 2017

kochi@, yes, and AFAICT the proper solution is to expose the CSSOrigin parameter added by r518913 (#c49) to extensions API chrome.tabs.insertCSS as cssOrigin option in the "details" object like it's done in Firefox: https://developer.mozilla.org/Add-ons/WebExtensions/API/tabs/insertCSS

Comment 56 by kochi@chromium.org, Dec 25 2017

Thanks - any estimate on this to be complete? or any blocker for this?

I'm in charge of deprecate/remove Shadow DOM V0, and this is one of the
blockers so would be interested in knowing feasibility / work estimate
on this.

Comment 57 by m.jeth...@eyeo.com, Dec 28 2017

kochi@ the changes for "cssOrigin" have been approved in crrev.com/c/778402

There are still some bugs in the user style sheets implementation; I have a couple of patches in review.
Labels: -Pri-1 Pri-2
m.jethani@ thanks for the update!
Project Member

Comment 60 by bugdroid1@chromium.org, Jan 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d1101adcfc3828ca582cc9197d4d5061ea65a6d

commit 1d1101adcfc3828ca582cc9197d4d5061ea65a6d
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Wed Jan 17 11:23:02 2018

Handle media queries in user style sheets

The implementation of user style sheets in crrev.com/c/641294 did not
account for media queries. As a result, once a user sheet's initial rule
set is generated, it is never updated based on changes in device type or
viewport dimensions. User rules intended for print, for example, have no
effect.

This patch fixes the issue by updating
StyleEngine::MediaQueryAffectingValueChanged to clear the rule sets for
any relevant user sheets and have them regenerated via
StyleEngine::UpdateActiveUserStyleSheets

CSSGlobalRuleSet now collects both user and scoped style features from
StyleEngine by calling StyleEngine::CollectFeaturesTo

BUG= 632009 

Change-Id: I38726d0e36f34cdeabcedb264d3e7c6bfac958f6
Reviewed-on: https://chromium-review.googlesource.com/789735
Commit-Queue: Manish Jethani <m.jethani@eyeo.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529707}
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/ActiveStyleSheets.cpp
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/ActiveStyleSheets.h
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/1d1101adcfc3828ca582cc9197d4d5061ea65a6d/third_party/WebKit/Source/core/css/TreeScopeStyleSheetCollection.cpp

Project Member

Comment 61 by bugdroid1@chromium.org, Jan 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9494d72752c2199295be01cbe066ff4b39a5a6ec

commit 9494d72752c2199295be01cbe066ff4b39a5a6ec
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Sat Jan 20 00:28:47 2018

Implement cssOrigin option for tabs.insertCSS

crrev.com/c/765642 makes user style sheets opt-in. In this patch we
implement the "cssOrigin" option for tabs.insertCSS, similar to Firefox.
If the value is set to "user", the CSS will be injected as a user style
sheet.

Style sheets specified in an extension's manifest are still injected as
author style sheets.

These are the code-level changes:

 1.  InjectDetails in extension_types.json now has a new "cssOrigin"
     property that takes the values "author" (default) and "user"
 2.  ScriptExecutor::ExecuteScript now takes a css_origin parameter of
     type base::Optional<CSSOrigin>
 3.  ExtensionMsg_ExecuteCode_Params now has a new member called
     css_origin of type base::Optional<CSSOrigin>, the value of which
     comes from the API call and is propagated down to
     blink::WebDocument

This API change is fully backwards compatible.

BUG= 632009 

Change-Id: Ia41ea4b917c7a9a4729e0a340ed7b3be43abdc11
Reviewed-on: https://chromium-review.googlesource.com/778402
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530686}
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/browser/extensions/execute_script_apitest.cc
[add] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/test/data/extensions/api_test/executescript/css_origin/b.js
[add] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/test/data/extensions/api_test/executescript/css_origin/manifest.json
[add] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/test/data/extensions/api_test/executescript/css_origin/test.html
[add] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/chrome/test/data/extensions/api_test/executescript/css_origin/test.js
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/browser/api/execute_code_function.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/browser/script_executor.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/browser/script_executor.h
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/common/api/extension_types.json
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/common/constants.h
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/common/extension_messages.h
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/programmatic_script_injector.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/programmatic_script_injector.h
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/script_injection.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/script_injector.h
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/user_script_injector.cc
[modify] https://crrev.com/9494d72752c2199295be01cbe066ff4b39a5a6ec/extensions/renderer/user_script_injector.h

Project Member

Comment 62 by bugdroid1@chromium.org, Jan 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5814e68d9cf56da28568855881c3d3944ada159e

commit 5814e68d9cf56da28568855881c3d3944ada159e
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Thu Jan 25 17:38:22 2018

Ignore any selectors at index 8192 or beyond

Since the selector index field in RuleData is only 13 bits, we can only
support lookups in the range 0-8191. We must ignore any selectors
outside this range.

BUG= 804179 , 632009 

Change-Id: I11c0acfe27813d98f24f72fb7e50f13b326a0e84
Reviewed-on: https://chromium-review.googlesource.com/883702
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Manish Jethani <m.jethani@eyeo.com>
Cr-Commit-Position: refs/heads/master@{#531931}
[modify] https://crrev.com/5814e68d9cf56da28568855881c3d3944ada159e/third_party/WebKit/Source/core/css/RuleSet.cpp

Can this be closed, as the upstream issue seems resolved?
I can still reproduce the issue with my adblocker on Chrome 67, so how is this resolved ?
Labels: OS-Chrome OS-Mac OS-Windows
Status: Fixed (was: Assigned)
Labels: -OS-Chrome
sscarl24@ you can try with a development build of Adblock Plus [1]

[1]: https://adblockplus.org/en/development-builds
by adblocker, I meant uBlock Origin dev builds.

static.kzar.co.uk##.hideme doesn't work but static.kzar.co.uk##.hideme:style(visibility: hidden !important) or static.kzar.co.uk##.hideme:style(visibility: collapse !important) works, so why doesn't the former work ?
sscarl24@ please see the issue description and the discussion above.

The first one doesn't work because it uses a CSS selector. The inline style="display: inline !important" setting overrides it. Now with user style sheets, the CSS selector will override the inline setting. For this to work though, uBlock Origin will have to start using user style sheets on Chromium. If you are a uBlock Origin user, please open an issue over there.

I suspect the second filter works because it sets the style attribute on the element directly, like Adblock Plus's "extended CSS selectors" [1]

[1]: https://adblockplus.org/filters#elemhide-emulation
Thank you for the explanation. Will do that.
How does feature detection work for this one? "chrome.extensionTypes" is undefined from background page, same for content script. 

From the JSON definition here: https://chromium.googlesource.com/chromium/src/+/master/extensions/common/api/_api_features.json
It seems that "chrome.extensionTypes" is for documentation only and is not available programmatically. Am I missing something? 

Sign in to add a comment