Status

()

Toolkit
WebExtensions: Frontend
P3
normal
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: andym, Assigned: evilpie, NeedInfo)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 months ago
We have tabs.insertCSS, but in bug 1226547 comment 1 its mentioned why we'd want a user CSS to be installed. This would be a CSS that:

"The latter likely needs to come with a "super-power" over its Chrome counterpart, which gives inserted stylesheets lower priority than content and does not honor !important: we should have an "overrideContent" flag or something like that, in order to prevent web pages from trumping extension-provided CSS rules."
(Reporter)

Comment 1

7 months ago
Currently tabs.insertCSS inserts a CSS as a USER_SHEET:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#193

If we allowed it to be inserted as an AUTHOR_SHEET:

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsIStyleSheetService.idl

Then it would be able to override the content sheets.
(Reporter)

Comment 2

7 months ago
Correction. It's currently inserted as AUTHOR_SHEET. This bug would allow insertion as a USER_SHEET.

Sorry for the confusion.

Updated

7 months ago
Priority: -- → P3
Whiteboard: triaged

Comment 3

7 months ago
This can be easily done by adding an `origin` property which defaults to `AUTHOR_SHEET` to the `details` argument of `tabs.insertCSS` and `tabs.removeCSS` (bug 1247455). This is also useful to Stylish-y add-ons, both to override `!important` style attributes and to style scrollers in the page.

Updated

6 months ago
See Also: → bug 1288917
(Assignee)

Updated

6 months ago
Assignee: nobody → evilpies
(Assignee)

Comment 4

6 months ago
Created attachment 8817543 [details] [diff] [review]
WIP
(Assignee)

Comment 5

5 months ago
I am open to naming suggestion, I don't feel like "origin" is really that great. Origin has another much more common meaning in the context of HTTP. Uh and we seem to share InjectDetails between JS and CSS.

Comment 6

5 months ago
I'm not really involved in the details here, but I like naming things.

What about 'definer' or 'cssDefiner'? It gets at the heart of what an AUTHOR or USER _SHEET means.
(Reporter)

Comment 7

5 months ago
I got to about the same point you did with a patch, which naughty me, I didn't add it to bugzilla. I called it type, purely because the SDK calls it that: https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_utils

But I agree origin is so overloaded with other meanings.
(Reporter)

Updated

5 months ago
webextensions: --- → +

Comment 8

5 months ago
(In reply to Matt from comment #6)
> I'm not really involved in the details here, but I like naming things.
> 
> What about 'definer' or 'cssDefiner'? It gets at the heart of what an AUTHOR
> or USER _SHEET means.

Or it could be something like 'precedence'. That's even more clear as to its role, being similar in meaning to "priority" but with more of a "which wins" connotation than priority's "what's the ordering?"
I don't think "priority" or "precedence" is appropriate because the cascading order is
1. user "!important"
2. author "!important"
3. author non-"!important"
4. user non-"!important"
.

Comment 10

5 months ago
Hmm. You make a good point. I'd forgotten that customizing the browser default and overriding a site's choices were two sides of the same mechanism.

I'd suggest "role" but that's already taken.

That said, I still think using an -er suffix sounds wrong unless used to name a class which implements some action. (eg. FooDownloader, BarCopier, BazHasher)
(Assignee)

Comment 11

5 months ago
I like "role"! In which sense is it taken? It's not a property of InjectDetails.

Comment 12

5 months ago
Huh. You're right. For some reason, my tired mind was telling me that screen/print/etc. were "roles" rather than "media types" and, if that were true, it'd have the same kind of problem as "origin".

However, I'd say that stylesheets and HTML element accessibility are distinct enough for such a common English word to be used by both without confusion.
(Assignee)

Comment 13

5 months ago
Created attachment 8821688 [details] [diff] [review]
Add "role" option for insertCSS/removeCSS
Attachment #8817543 - Attachment is obsolete: true
Attachment #8821688 - Flags: review?(aswan)

Comment 14

5 months ago
Comment on attachment 8821688 [details] [diff] [review]
Add "role" option for insertCSS/removeCSS

Redirecting to Kris.  My only minor comment is that you've misspelled the name of the new property in the patch subject (rule/role)
Attachment #8821688 - Flags: review?(aswan) → review?(kmaglione+bmo)
(Assignee)

Updated

5 months ago
Attachment #8821688 - Attachment description: Add "rule" option for insertCSS/removeCSS → Add "role" option for insertCSS/removeCSS
Keywords: dev-doc-needed
Comment on attachment 8821688 [details] [diff] [review]
Add "role" option for insertCSS/removeCSS

Review of attachment 8821688 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with "role" changed to "origin".

Thanks!

::: browser/components/extensions/ext-tabs.js
@@ +844,4 @@
>          } else {
>            options.run_at = "document_idle";
>          }
> +        if (details.role !== null) {

This CSS spec calls this the origin, not the role:

https://www.w3.org/TR/CSS2/cascade.html#cascade
Attachment #8821688 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 16

5 months ago
Please see the previous discussion, I don't think origin is a good name to use.
I'm still not comfortable defining a whole new term for this, when one already exists in the spec... How about something like "originRole"?
(Assignee)

Comment 18

5 months ago
We agreed to cssOrigin on IRC.

Comment 19

5 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68402c8f7fd2
webext: Add cssOrigin option to insertCSS/removeCSS. r=kmag

Comment 20

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68402c8f7fd2
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I've added this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/insertCSS

Let me know if this covers it.
Flags: needinfo?(evilpies)
(Assignee)

Comment 22

3 months ago
The text looks good. don't forget to add a new row to the compatibility table and update https://developer.mozilla.org/en-US/Firefox/Releases/53.
Flags: needinfo?(evilpies)
Keywords: dev-doc-needed → dev-doc-complete
Hey, IIUC the cssOrigin arguments should also be added to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/removeCSS and maybe https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extensionTypes/InjectDetails (indicating it's specific to *CSS functions)?
Flags: needinfo?(wbamberg)
You need to log in before you can comment on or make changes to this bug.