Closed Bug 1233127 Opened 5 years ago Closed 5 years ago

Use createProcessingInstruction or a <link> in theme switcher to apply stylesheets instead of addon sdk

Categories

(DevTools :: Framework, defect)

45 Branch
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

There's a funny property when calling StylesheetUtils.loadSheet in which the loaded sheet applies after other sheets (even if it's set as author).  See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1224932#c2.

When working on the devtools.html I found calling createProcessingInstruction works just as well and doesn't have this problem
Whiteboard: [devtools-html]
History of changes to get this to work in content priv is here: https://github.com/joewalker/devtools.html/blob/master/client/shared/theme-switching.js
Attached patch stylesheet-loading.patch (obsolete) — Splinter Review
Need to confirm this works for HTML pages (works fine for xhtml and xul).  Patrick, can you try with this patch applied and see if it's working for you?  Especially the floating scrollbars bit in the dark theme.
Attachment #8699094 - Flags: feedback?(pbrosset)
This should work on HTML pages fine as per the 'Exceptions' section here: https://developer.mozilla.org/en-US/docs/Web/API/Document/createProcessingInstruction.   I've expanded the test coverage to make sure the sheets are loading also.
Attachment #8699094 - Attachment is obsolete: true
Attachment #8699094 - Flags: feedback?(pbrosset)
Hmm apparently the scrollbars file still needs to be loaded as an agent sheet or else it doesn't apply to the page :(
Comment on attachment 8699150 [details]
MozReview Request: Bug 1233127 - Use createProcessingIntruction instead of addon-sdk to load stylesheets in tools;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28185/diff/1-2/
Comment on attachment 8699150 [details]
MozReview Request: Bug 1233127 - Use createProcessingIntruction instead of addon-sdk to load stylesheets in tools;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28185/diff/2-3/
There was a test that was checking the DOM immediately after clicking on a theme checkbox to make sure the theme was applied.  Since the old method of loading was synchronous this used to work but now it was failing.  So I needed to add code to keep track of the load / error of the sheet and notify the window once it happens so that the test can proceed with checking the DOM.  Also reworked that test to use add_task and be e10s friendly while I was at it.

New try push:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f79eb97b251
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Need to confirm this works for HTML pages (works fine for xhtml and xul). 
> Patrick, can you try with this patch applied and see if it's working for
> you?  Especially the floating scrollbars bit in the dark theme.
TIL processing instructions!
Interestingly, this doesn't seem to work for me when I add a <?xml-stylesheet ... ?> processing instruction to an HTML document. But adding it dynamically with document.createProcessingInstruction does work though. So it should be ok for HTML documents.
Comment on attachment 8699150 [details]
MozReview Request: Bug 1233127 - Use createProcessingIntruction instead of addon-sdk to load stylesheets in tools;r=pbrosset

https://reviewboard.mozilla.org/r/28185/#review25305

This works well locally.
I've tested in both light and dark theme on windows, and confirmed that I can remove my quick fix in bug 1224932.
And it also has the benefit of showing the theme files as author CSS in the browser-toolbox's rule-view, which is convenient.

::: devtools/client/framework/test/browser_toolbox_theme_registration.js:10
(Diff revision 3)
> -{
> +  let tab = yield addTab("data:text/html,test for dynamically registering and unregistering themes");

Maybe move the "Test for dynamically registering and unregistering themes" as a line comment at the top of the test file.
It's way more useful there I believe, because that's the first thing you read when opening the test and tells you what the test is about.
Having it show in the URL bar of the test window is useless cause you never have the time to look at it.

"data:text/html;charset=utf-8,test" would be enough

::: devtools/client/framework/test/browser_toolbox_theme_registration.js:15
(Diff revision 3)
> -    gBrowser.selectedBrowser.removeEventListener(evt.type, onLoad, true);
> +    gDevTools.once("theme-registered", (e,themeId) => {

nit: space after comma: `e, themeId`

::: devtools/client/framework/test/browser_toolbox_theme_registration.js:67
(Diff revision 3)
> +  yield new Promise(resolve => {

Maybe move this to a helper function at the bottom of the test, or in head.js

::: devtools/client/shared/test/browser_theme_switching.js:19
(Diff revision 3)
> -  ok(root.classList.contains(className), ":root has " + className + " class (current theme)");
> +  ok(root.classList.contains(className),":root has " + className + " class (current theme)");

Why remove the space after the comma here.
Also maybe split to 2 lines to fit in 80 chars.

::: devtools/client/shared/theme-switching.js:39
(Diff revision 3)
> +        resolve();

If there's ever a chance that a stylesheet doesn't load correctly, I bet we'll want to know why. So maybe we should reject here instead with the error message (and further below, when we wait for the resolution, we should have a rejection handler).

::: devtools/client/shared/theme-switching.js:43
(Diff revision 3)
> +    return {styleSheet,loadPromise};

nit: space after the comma `styleSheet, loadPromise`

::: devtools/client/shared/theme-switching.js:129
(Diff revision 3)
> +    Promise.all(loadEvents).then(notifyWindow);

If you do the change to reject when stylesheets don't load, then you should also `console.error` the errors here.
Attachment #8699150 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #10)
> Comment on attachment 8699150 [details]
> MozReview Request: Bug 1233127 - Use createProcessingIntruction instead of
> addon-sdk to load stylesheets in tools;r=pbrosset
> 
> https://reviewboard.mozilla.org/r/28185/#review25305
> 
> This works well locally.
> I've tested in both light and dark theme on windows, and confirmed that I
> can remove my quick fix in bug 1224932.
> And it also has the benefit of showing the theme files as author CSS in the
> browser-toolbox's rule-view, which is convenient.
> 

Thanks for the review

> ::: devtools/client/framework/test/browser_toolbox_theme_registration.js:10
> (Diff revision 3)
> > -{
> > +  let tab = yield addTab("data:text/html,test for dynamically registering and unregistering themes");
> 
> Maybe move the "Test for dynamically registering and unregistering themes"
> as a line comment at the top of the test file.
> It's way more useful there I believe, because that's the first thing you
> read when opening the test and tells you what the test is about.
> Having it show in the URL bar of the test window is useless cause you never
> have the time to look at it.
> 
> "data:text/html;charset=utf-8,test" would be enough
> 

Updated

> ::: devtools/client/framework/test/browser_toolbox_theme_registration.js:67
> (Diff revision 3)
> > +  yield new Promise(resolve => {
> 
> Maybe move this to a helper function at the bottom of the test, or in head.js
> 

Realized we already have a helper in shared-head for this, so we can do `yield once(panelWin, "theme-switch-complete");`

> ::: devtools/client/shared/theme-switching.js:39
> (Diff revision 3)
> > +        resolve();
> 
> If there's ever a chance that a stylesheet doesn't load correctly, I bet
> we'll want to know why. So maybe we should reject here instead with the
> error message (and further below, when we wait for the resolution, we should
> have a rejection handler).

Yeah that makes sense
https://hg.mozilla.org/mozilla-central/rev/e1ce5b4fa814
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1233689
Depends on: 1234801, 1236106
Depends on: 1237788
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.