Closed Bug 634764 Opened 13 years ago Closed 12 years ago

Allow adding CSS stylesheets to page with page-mod API

Categories

(Add-on SDK Graveyard :: General, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: harth, Assigned: zer0)

References

Details

Attachments

(2 files, 1 obsolete file)

Right now you can attach scripts to webpages with pageMod.contentScript and pageMod.contentScriptFile, there should be a pageMod.contentStyleSheet/pageMod.contentStyleSheetFile that lets you attach a CSS file to the page.

You can technically achieve this using just scripting and some message passing, but I think attaching styles to a page is a common enough use case to warrant its own API in page-mod.
Here is a preliminary work.

Myk, I'll continue working on this (mainly adding docs) if you think we could ship this in the next beta. 
Having said that, I have two concerns about this patch:
1/ There may be some problem with CSS injection on XML documents. I used a HTML pattern to insert CSS, but there is an alternative XML way to add them that may be mandatory on some documents (XUL for sure, may be some very strict XHTML).
2/ We may expect that CSS are removed on unload/destroy. But if we implement this feature, we have to think about such pattern for contentScript too. Consequently, this patch is going to be much more complex!

Recent discussion on IRC and on group:
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/666bccc8cc887cd5
http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/515e45823f1db446
... tend to say that this feature is a common request.
Attachment #513974 - Flags: feedback?(myk)
Comment on attachment 513974 [details] [diff] [review]
Add contentStylesheet contentStylesheetFile on page-mod

(In reply to comment #1)
> Myk, I'll continue working on this (mainly adding docs) if you think we could
> ship this in the next beta. 

Yup, I think we can.


> 1/ There may be some problem with CSS injection on XML documents. I used a HTML
> pattern to insert CSS, but there is an alternative XML way to add them that may
> be mandatory on some documents (XUL for sure, may be some very strict XHTML).

I'm not worried about such documents, which represent a very small portion of the web.


> Another option is to use user stylesheets to insert the rules.

Right.  The page-mod match pattern syntax was designed to be compatible with @-moz-document rules, which makes it possible to use user stylesheets (although I seem to recall some discussion about expanding the match pattern syntax, which would break the user stylesheets approach).


> 2/ We may expect that CSS are removed on unload/destroy. But if we implement
> this feature, we have to think about such pattern for contentScript too.
> Consequently, this patch is going to be much more complex!

I'm not sure whether it's documented or not, but it is intentional behavior that page mods remain attached after their addon is uninstalled until the pages they mod are reloaded.  Ideally they would be detached on addon uninstall, but there's no way to know what changes a page mod has made and force it to unmake them.

So this is ok.
Attachment #513974 - Flags: feedback?(myk) → feedback+
I've improved API to handle arrays of files or css texts,
and added documentation with a new specific example about this feature.
Attachment #513974 - Attachment is obsolete: true
Attachment #519410 - Flags: review?(myk)
I have created a git branch for this https://github.com/mcepl/addon-sdk/tree/bug634764_contentStylesheet, and I can make a pull request if it makes anything more simple (and you don't mind loosing a fame of fixing this in git log), but

Test fails (on jetpack 09d5b130 and FF4.0)

error: TEST FAILED: test-page-mod.testPageModCss (failure)
error: fail: PageMod contentStylesheetFile worked (100 != 120)
info: Traceback (most recent call last):
  File "resource://testpkgs-addon-kit-tests/pagemod-test-helpers.js", line 40, i
n onPageLoad
    testCallback(b.contentWindow.wrappedJSObject, function done() {
  File "resource://testpkgs-addon-kit-tests/test-page-mod.js", line 225, in null
    "PageMod contentStylesheetFile worked"
  File "resource://testpkgs-api-utils-lib/unit-test.js", line 197, in assertEqua
l
    this.fail(message);
  File "resource://testpkgs-api-utils-lib/unit-test.js", line 115, in fail
    console.trace();
Comment on attachment 519410 [details] [diff] [review]
Implementation + documentation

I haven't done the line-by-line review yet, but I read through this patch and noticed a few general issues:

1. contentStylesheet and contentStylesheetFile should be simply contentStyle and contentStyleFile.

2. Style is attached in _onAttach, but the point in time at which that method is called during the page load depends on contentScriptWhen and can happen as soon as content-document-element-created, which is before the <head> element is created (and thus before it is possible to attach stylesheets to the page).  Style should be attached when the time is right, regardless of the value of contentScriptWhen; ideally, this would be right after </head>, but realistically it can be on DOMContentLoaded.

3. Presumably the same origin policy prevents pages from introspecting the added stylesheet files, although perhaps they are able to iterate them.  It would be good to verify the behavior.  And it would also be good to insert contentStyle via a URL from a different origin, so it too is protected from being accessed by the page.
Attachment #519410 - Flags: review?(myk) → review-
I'd also like to get dbaron's take on inserting <link> and <style> elements into modified pages versus using user stylesheets.

David: the Page Mod API in Add-on SDK lets addons modify pages with script à la Greasemonkey.  This bug is about adding support for modifying pages with style as well.  The two implementation approaches we have identified are:

1. Writing addon style into the user stylesheet.

2. Injecting <link> (and possibly <style>, although probably not) elements into the pages being modified.

The user stylesheet approach seems simpler to implement and relies more on existing functionality, but it would prevent us from expanding the match pattern syntax for identifying the pages to be modified (f.e. in bug 627386).

The <link> injection approach gives us match pattern syntax flexibility, but its privacy characteristics are unclear.

Any thoughts on the implications of these approaches or suggestions for avenues of further exploration?
Priority: -- → P2
Target Milestone: --- → 1.0
I talked to David last week, and he recommended that we implement style mods using user stylesheets rather than document stylesheets, for several reasons.

In particular, user stylesheets preserve the cascade, by which page author rules override user rules (unless those user rules are !important, in which case they override the page author rules).  They thus conform to the CSS specifications for overriding page style.

User stylesheets are also better protected from introspection by the page.

User stylesheets don't currently support regex-based site matching, which we want to add to page mods over in bug 627386, but David suggested that adding such support would not be particularly complicated, and he is amenable to making that change.

So we should implement this using user stylesheets.
And in particular, I think the most important cascading issue is that if you add sheets to the author level, then you have to care about how the specificity of individual selectors in the sheet being added relates to the specificity of individual selectors in the page -- which doesn't seem like something the (API) user should have to care about.

Also -- you shouldn't need to write into "the" user style sheet -- you can use http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIStyleSheetService.idl
(In reply to comment #7)
> So we should implement this using user stylesheets.

How will this stack against the REAL user’s stylesheets? Will it be still possible for the user to add his own stylesheets to the page, and/or use extensions like Stylish?
(In reply to comment #9)
> How will this stack against the REAL user’s stylesheets? Will it be still
> possible for the user to add his own stylesheets to the page, and/or use
> extensions like Stylish?

userContent.css will have higher precedence than the user stylesheets this API adds, so users will be able to add styles that override this API's styles.

They will also be able to use addons like Stylish, although the relative ordering of two user stylesheets added via nsIStyleSheetService is undefined, so if such addons also use that service, rules added by this API might override rules they add.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIStyleSheetService.idl#56
dbaron: is there already a bug on implementing regex URL matching in user stylesheets, or should I file one?
Depends on: 398962
Could we get this patch unbitrotten? It doesn't apply at all against commit 58b365d6 (HEAD on the github repo as of today).
(automatic reprioritization of 1.0 bugs)
Priority: P2 → P1
Target Milestone: 1.0 → 1.1
Assignee: nobody → poirot.alex
Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)
Severity: normal → enhancement
Re-prioritizing all 1.1-targeted feature requests to 1.2.
Target Milestone: 1.1 → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
Is this still on the table for the future?  Reading up the thread, it sounds like a solution was agreed upon, quite a while ago, but no activity since then!
Comment on attachment 604081 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/366#

><!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla/addon-sdk/pull/366#"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla/addon-sdk/pull/366#">https://github.com/mozilla/addon-sdk/pull/366#</a>, or wait 5 seconds to be redirected there automatically.</p>
Attachment #604081 - Flags: review?(poirot.alex)
Assignee: poirot.alex → zer0
Comment on attachment 604081 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/366#

I have picked it up from @ochameau as he's review queue is pretty big.
It's almost there but there are small things to be addressed.
Attachment #604081 - Flags: review?(poirot.alex) → review-
Comment on attachment 604081 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/366#

Looks good, few minor nits, but no more review required!
Attachment #604081 - Flags: review- → review+
Irakli, I updated the pull request with the reviews' comments, the only thing I didn't modify was the include's rule for consistency – I added a comment about that.

If you feel it's okay, could you land it? I do not have the access on Mozilla repo to do that.

Thanks!
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/cb5537059c267234704b3832a84f44cc88de0b78
Merge pull request #366 from ZER0/bug634764

bug 634764 – Added `contentStyle` and `contentStyleFile` to PageMod's options
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.