Last Comment Bug 634764 - Allow adding CSS stylesheets to page with page-mod API
: Allow adding CSS stylesheets to page with page-mod API
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 enhancement with 2 votes (vote)
: ---
Assigned To: Matteo Ferretti [:zer0] [:matteo]
:
Mentors:
Depends on: 398962 738372
Blocks: 627386
  Show dependency treegraph
 
Reported: 2011-02-16 15:32 PST by Heather Arthur [:harth]
Modified: 2012-03-22 16:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add contentStylesheet contentStylesheetFile on page-mod (3.27 KB, patch)
2011-02-21 05:23 PST, Alexandre Poirot [:ochameau]
myk: feedback+
Details | Diff | Splinter Review
Implementation + documentation (7.21 KB, patch)
2011-03-15 08:26 PDT, Alexandre Poirot [:ochameau]
myk: review-
Details | Diff | Splinter Review
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/366# (358 bytes, text/html)
2012-03-08 08:38 PST, Matteo Ferretti [:zer0] [:matteo]
rFobic: review+
Details

Description Heather Arthur [:harth] 2011-02-16 15:32:22 PST
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.
Comment 1 Alexandre Poirot [:ochameau] 2011-02-21 05:23:29 PST
Created attachment 513974 [details] [diff] [review]
Add contentStylesheet contentStylesheetFile on 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.
Comment 2 Myk Melez [:myk] [@mykmelez] 2011-03-14 00:30:40 PDT
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.
Comment 3 Alexandre Poirot [:ochameau] 2011-03-15 08:26:03 PDT
Created attachment 519410 [details] [diff] [review]
Implementation + documentation

I've improved API to handle arrays of files or css texts,
and added documentation with a new specific example about this feature.
Comment 4 Matěj Cepl 2011-03-26 02:54:45 PDT
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 5 Myk Melez [:myk] [@mykmelez] 2011-03-27 22:14:06 PDT
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.
Comment 6 Myk Melez [:myk] [@mykmelez] 2011-03-27 22:21:23 PDT
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?
Comment 7 Myk Melez [:myk] [@mykmelez] 2011-04-11 13:48:25 PDT
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.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-04-11 16:05:19 PDT
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
Comment 9 Matěj Cepl 2011-04-12 23:18:43 PDT
(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?
Comment 10 Myk Melez [:myk] [@mykmelez] 2011-04-13 08:23:55 PDT
(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
Comment 11 Myk Melez [:myk] [@mykmelez] 2011-04-19 13:40:26 PDT
dbaron: is there already a bug on implementing regex URL matching in user stylesheets, or should I file one?
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-04-19 14:32:21 PDT
Bug 398962.
Comment 13 Matěj Cepl 2011-04-29 03:07:04 PDT
Could we get this patch unbitrotten? It doesn't apply at all against commit 58b365d6 (HEAD on the github repo as of today).
Comment 14 Myk Melez [:myk] [@mykmelez] 2011-06-15 12:55:30 PDT
(automatic reprioritization of 1.0 bugs)
Comment 15 Wes Kocher (:KWierso) 2011-08-08 11:57:38 PDT
Marking anything that potentially looks like a feature request as "enhancement", sorry for the bugspam. :)
Comment 16 Wes Kocher (:KWierso) 2011-08-10 10:38:50 PDT
Re-prioritizing all 1.1-targeted feature requests to 1.2.
Comment 17 Wes Kocher (:KWierso) 2011-09-08 12:02:42 PDT
(Pushing all open bugs to the --- milestone for the new triage system)
Comment 18 Tim Martin 2012-02-13 15:40:03 PST
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 19 Matteo Ferretti [:zer0] [:matteo] 2012-03-08 08:38:14 PST
Created attachment 604081 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/366#

Pointer to Github pull-request
Comment 20 Matteo Ferretti [:zer0] [:matteo] 2012-03-08 08:39:10 PST
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>
Comment 21 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-03-08 10:07:11 PST
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.
Comment 22 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2012-03-14 10:56:35 PDT
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!
Comment 23 Matteo Ferretti [:zer0] [:matteo] 2012-03-22 04:52:33 PDT
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!
Comment 24 [github robot] 2012-03-22 10:58:07 PDT
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
Comment 25 [github robot] 2012-03-22 16:40:04 PDT
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/362667ee7568ffc79a13e0f1ca8fe3a33de3ed31
Merge pull request #383 from ZER0/bug634764

fix bug 634764 r=@gozala

Note You need to log in before you can comment on or make changes to this bug.