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)
Add-on SDK Graveyard
General
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.
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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+
Comment 3•13 years ago
|
||
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)
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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-
Comment 6•13 years ago
|
||
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?
Updated•13 years ago
|
Priority: -- → P2
Target Milestone: --- → 1.0
Comment 7•13 years ago
|
||
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
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
dbaron: is there already a bug on implementing regex URL matching in user stylesheets, or should I file one?
Comment 13•13 years ago
|
||
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•13 years ago
|
||
(automatic reprioritization of 1.0 bugs)
Priority: P2 → P1
Target Milestone: 1.0 → 1.1
Updated•13 years ago
|
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 → ---
Comment 18•12 years ago
|
||
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!
Assignee | ||
Comment 19•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 20•12 years ago
|
||
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>
Assignee | ||
Updated•12 years ago
|
Attachment #604081 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•12 years ago
|
Assignee: poirot.alex → zer0
Comment 21•12 years ago
|
||
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 22•12 years ago
|
||
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+
Assignee | ||
Comment 23•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 738372
Comment 25•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•