Closed Bug 1141326 Opened 5 years ago Closed 5 years ago

DevEdition theme has flash of unstyled content

Categories

(Firefox :: Theme, defect, P3)

All
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 3 obsolete files)

When initially loading the devedition theme, there is a brief delay while the stylesheet is loading.  During this time the normal theme is applied, so the flash is especially noticeable when the dark devedition theme is applied.
We could load the stylesheet before onLoad for the browser (with disabled=true) set.  It will take some reorganizing that will be easier after we convert to a lightweight theme in bug 1094821.
Depends on: 1094821
Hardware: x86 → All
Guessing this now depends on bug 1148996, too.
Depends on: 1148996
Whiteboard: [devedition-40][difficulty=easy]
Attached patch devedition-fouc-approach-1.patch (obsolete) — Splinter Review
This approach runs some script at the bottom of browser.js to create the processing instruction - that way it loads before gBrowserInit.onLoad fires.

I have another approach that involves adding the stylesheet directly into browser.xul.  I'll upload that next.
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Attached patch devedition-fouc-2.patch (obsolete) — Splinter Review
I actually prefer this approach in terms of it simplifying the code and not having to execute code inline in browser.js.  Although it does have a sort-of-hack in which I set media="none" on the stylesheet (since disabled isn't valid in the markup for some reason).

Gijs, what do you think?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8589964 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8589964 [details] [diff] [review]
devedition-fouc-2.patch

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

::: browser/base/content/browser-devedition.js
@@ +23,5 @@
>    init: function () {
> +
> +    for (let i = document.styleSheets.length - 1; i >= 0; i--) {
> +      let sheet = document.styleSheets[i];
> +      if (sheet.href == "chrome://browser/skin/devedition.css") {

Could use this.styleSheetLocation instead of the hardcoded string
Comment on attachment 8589964 [details] [diff] [review]
devedition-fouc-2.patch

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

I'm worried that this will have perf impact everywhere (not just devedition) because of the now-default-loaded stylesheet (at least, I'm assuming the extra load will delay the load event etc.). I think I would prefer not to do that.

Can you do trypushes to quantify this?

If the impact is significant, it might make sense to do this behind an ifdef only on aurora/devedition, where it is more likely to be used? Although maybe the lwt work means that it's possible to use the theme elsewhere and that might mean this isn't a good argument anymore? Not sure...
Attachment #8589964 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8589964 [details] [diff] [review]
> devedition-fouc-2.patch
> 
> Review of attachment 8589964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm worried that this will have perf impact everywhere (not just devedition)
> because of the now-default-loaded stylesheet (at least, I'm assuming the
> extra load will delay the load event etc.). I think I would prefer not to do
> that.
> 
> Can you do trypushes to quantify this?
> 
Fair enough.. here is an ongoing try push with talos tests enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8285d37ed839

> If the impact is significant, it might make sense to do this behind an ifdef
> only on aurora/devedition, where it is more likely to be used? Although
> maybe the lwt work means that it's possible to use the theme elsewhere and
> that might mean this isn't a good argument anymore? Not sure...

We are already ifdeffing the existence of the lw theme for non-release builds, so we could definitely do the same for the stylesheet [0].  In theory we could support beta/release with the theme, but until we have a strong reason to do so it's not available.

The tricky part if we excluded the stylesheet is that the test will fail in release builds, so maybe approach 1 is easier to deal with as far as having a function that we can call to do this eager loading.  So we could only call it immediately in browser.js only if it looks like the DE theme is going to be applied - otherwise initialize it only when it's about to be enabled (which will handle the case of the theme being applied from addons manager, or during test runs).

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#723
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Comment on attachment 8589964 [details] [diff] [review]
> > devedition-fouc-2.patch
> > 
> > Review of attachment 8589964 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm worried that this will have perf impact everywhere (not just devedition)
> > because of the now-default-loaded stylesheet (at least, I'm assuming the
> > extra load will delay the load event etc.). I think I would prefer not to do
> > that.
> > 
> > Can you do trypushes to quantify this?
> > 
> Fair enough.. here is an ongoing try push with talos tests enabled:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8285d37ed839

I pushed a baseline trypush: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=34010f830d1e

We'll still need to retrigger the talos results on it (probably just the 'o' runs, but we could do the 'tp' ones to be sure).


> > If the impact is significant, it might make sense to do this behind an ifdef
> > only on aurora/devedition, where it is more likely to be used? Although
> > maybe the lwt work means that it's possible to use the theme elsewhere and
> > that might mean this isn't a good argument anymore? Not sure...
> 
> We are already ifdeffing the existence of the lw theme for non-release
> builds, so we could definitely do the same for the stylesheet [0].  In
> theory we could support beta/release with the theme, but until we have a
> strong reason to do so it's not available.
> 
> The tricky part if we excluded the stylesheet is that the test will fail in
> release builds, so maybe approach 1 is easier to deal with as far as having
> a function that we can call to do this eager loading. So we could only call
> it immediately in browser.js only if it looks like the DE theme is going to
> be applied

But we do this already, right? DevEdition.init() is called from onLoad. Does the FOUC only happen for loading devedition while the browser is already running?
(In reply to :Gijs Kruitbosch from comment #9)
> > > If the impact is significant, it might make sense to do this behind an ifdef
> > > only on aurora/devedition, where it is more likely to be used? Although
> > > maybe the lwt work means that it's possible to use the theme elsewhere and
> > > that might mean this isn't a good argument anymore? Not sure...
> > 
> > We are already ifdeffing the existence of the lw theme for non-release
> > builds, so we could definitely do the same for the stylesheet [0].  In
> > theory we could support beta/release with the theme, but until we have a
> > strong reason to do so it's not available.
> > 
> > The tricky part if we excluded the stylesheet is that the test will fail in
> > release builds, so maybe approach 1 is easier to deal with as far as having
> > a function that we can call to do this eager loading. So we could only call
> > it immediately in browser.js only if it looks like the DE theme is going to
> > be applied
> 
> But we do this already, right? DevEdition.init() is called from onLoad. Does
> the FOUC only happen for loading devedition while the browser is already
> running?

We currently add the sheet onLoad (which causes the FOUC on startup because we don't start loading the sheet until after the browse has finished loading).  Approach 1 adds the sheet at the bottom of browser.js (before onLoad, just directly executing in the script).  We could modify that approach by only doing it before onLoad if DevEdition._isThemeCurrentlyApplied.

One thing about approach 1 that's weird is that it ends up executing also in the hiddenDOMWindow.  I'm not sure what the best way to differentiate at that point in time is, since gBrowser doesn't yet exist.
(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > > > If the impact is significant, it might make sense to do this behind an ifdef
> > > > only on aurora/devedition, where it is more likely to be used? Although
> > > > maybe the lwt work means that it's possible to use the theme elsewhere and
> > > > that might mean this isn't a good argument anymore? Not sure...
> > > 
> > > We are already ifdeffing the existence of the lw theme for non-release
> > > builds, so we could definitely do the same for the stylesheet [0].  In
> > > theory we could support beta/release with the theme, but until we have a
> > > strong reason to do so it's not available.
> > > 
> > > The tricky part if we excluded the stylesheet is that the test will fail in
> > > release builds, so maybe approach 1 is easier to deal with as far as having
> > > a function that we can call to do this eager loading. So we could only call
> > > it immediately in browser.js only if it looks like the DE theme is going to
> > > be applied
> > 
> > But we do this already, right? DevEdition.init() is called from onLoad. Does
> > the FOUC only happen for loading devedition while the browser is already
> > running?
> 
> We currently add the sheet onLoad (which causes the FOUC on startup because
> we don't start loading the sheet until after the browse has finished
> loading).  Approach 1 adds the sheet at the bottom of browser.js (before
> onLoad, just directly executing in the script).  We could modify that
> approach by only doing it before onLoad if
> DevEdition._isThemeCurrentlyApplied.
> 
> One thing about approach 1 that's weird is that it ends up executing also in
> the hiddenDOMWindow.  I'm not sure what the best way to differentiate at
> that point in time is, since gBrowser doesn't yet exist.

Compare window with Services.appShell.hiddenDOMWindow ? :-)
Attached patch devedition-fouc.patch (obsolete) — Splinter Review
I think loading the stylesheet in js is going to give us more control over your concerns in comment 7.  I've extracted the stylesheet loading into a separate function, which is called at these times:

* Immediately in browser.js if the DE theme is going to be applied
* Inside _toggleStyleSheet if the sheet is going to be enabled and it hasn't been called yet.  This function is called in a lightweight-theme-styling-update event

It preserves the existing behavior in the case of a non-DE-themed browser.  Which is that the stylesheet never gets created.

Assuming you agree with this plan, my main question/concern has to do with the placement of the call to DevEdition.createStyleSheet in browser.js.  Maybe there is a different callback that happens earlier where it could run, or it could be put into browser-devedition.js and get preprocessed into browser.js at that point.
Attachment #8589929 - Attachment is obsolete: true
Attachment #8589964 - Attachment is obsolete: true
Attachment #8590305 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8590305 [details] [diff] [review]
devedition-fouc.patch

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

::: browser/base/content/browser-devedition.js
@@ +34,5 @@
>  
> +  createStyleSheet: function() {
> +    let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
> +    this.styleSheet = document.createProcessingInstruction(
> +      'xml-stylesheet', styleSheetAttr);

Nit: double quotes.

::: browser/base/content/browser.js
@@ +7808,5 @@
> +// then preload it now.  This prevents a flash of unstyled content where the
> +// normal theme is applied while the DevEdition stylesheet is loading.
> +if (this != Services.appShell.hiddenDOMWindow && DevEdition.isThemeCurrentlyApplied) {
> +  DevEdition.createStyleSheet();
> +}

Right, I think including this toplevel in browser-devedition.js is fine.

Should this be ifdef'd for non-release builds, then, per your earlier comment that the theme is never available on beta/release?
Attachment #8590305 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #14)
> 
> Should this be ifdef'd for non-release builds, then, per your earlier
> comment that the theme is never available on beta/release?

May as well. *In theory* devedition should never be returned as the LWTM.currentTheme in release, but it doesn't hurt anything to preprocess this out.
Moved the call to createStyleSheet into browser-devedition.js and preprocessed it with #ifndef RELEASE_BUILD.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0a6a2d1c9cd
Along with a reference one for talos comparison: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27c34c547f11.  Although really this shouldn't have an effect on talos since the DE theme is preffed off there.
Attachment #8590305 - Attachment is obsolete: true
Attachment #8590479 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8590479 [details] [diff] [review]
devedition-fouc.patch

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

r=me!
Attachment #8590479 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/484fe7759dd4
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/484fe7759dd4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
You need to log in before you can comment on or make changes to this bug.