1458997 - [Dark Theme] Regression: Toolbar flickers for a short moment when starting Nightly

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mehmet]

Attachment: Dark Theme Toolbar Flicker.mov

Nightly 61.0a1 (2018-05-03) (64-Bit)
macOS 10.12.6

STR:

1.) Use the official Dark Theme
2.) Start Nightly
3.) Take a look at the Toolbar, when Nightly starts

Actual: The Toolbar flickers for a short moment.

Expected: No flicker.

This is a regression in latest Nightly.

A screencast is attached.

Thanks,
Mehmet

Comment 1

[Mehmet]

Attachment: Screenshot.png

+Screenshot of that moment.

Comment 2

[Mehmet]

Here is my manual bisect:

Good: 2018-03-16-10-01-32-mozilla-central

 Bad: 2018-03-16-22-01-24-mozilla-central

From the pushlog in this range, maybe caused by https://hg.mozilla.org/mozilla-central/rev/27d65d2c5909 ?

Dão: What do you think? Thanks for checking :)

Comment 3

[Dão Gottwald [:dao]]

(In reply to Mehmet from comment #2)
> Here is my manual bisect:
> 
> Good: 2018-03-16-10-01-32-mozilla-central
> 
>  Bad: 2018-03-16-22-01-24-mozilla-central
> 
> From the pushlog in this range, maybe caused by
> https://hg.mozilla.org/mozilla-central/rev/27d65d2c5909 ?
> 
> Dão: What do you think? Thanks for checking :)

Yeah, I suspected it would be that bug. Thanks!

Comment 4

[Tim Nguyen :ntim]

We could add transition: none; to the compacttheme.inc.css stylesheet.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #4)
> We could add transition: none; to the compacttheme.inc.css stylesheet.

I expect that not only the Dark and Light themes are affected here.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Review commit: https://reviewboard.mozilla.org/r/242144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242144/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

https://reviewboard.mozilla.org/r/242144/#review247970

::: browser/themes/shared/browser.inc.css:21
(Diff revision 1)
>  
>  :root[extradragspace][tabsintitlebar]:not([inFullscreen]) {
>    --space-above-tabbar: 8px;
>  }
>  
> -:root:-moz-lwtheme {
> +:root[sessionrestored] :root:-moz-lwtheme {

broken selector

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242144/diff/1-2/

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

https://reviewboard.mozilla.org/r/242144/#review247980

::: browser/themes/shared/browser.inc.css:57
(Diff revision 2)
> +}
> +
>  #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar)[collapsed=true] {
>    min-height: 0.1px;
>    max-height: 0;
>    transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;

This is incorrectly being overridden by your new rule.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

https://reviewboard.mozilla.org/r/242144/#review247980

> This is incorrectly being overridden by your new rule.

That was intentional, to override the rule with the extra transition properties a bit later. Though I could remove the old one since we don't necessarily need this one so early anyways.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242144/diff/2-3/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> > This is incorrectly being overridden by your new rule.
> 
> That was intentional, to override the rule with the extra transition
> properties a bit later. Though I could remove the old one since we don't
> necessarily need this one so early anyways.

Nevermind I see what you were talking about. This is now fixed.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

https://reviewboard.mozilla.org/r/242144/#review247994

::: browser/themes/shared/browser.inc.css:53
(Diff revision 3)
> +:root[sessionrestored] #navigator-toolbox > toolbar:not(#toolbar-menubar):not(#TabsToolbar):not(#nav-bar)[collapsed=true] {
>    min-height: 0.1px;
>    max-height: 0;

This doesn't look right, this means the hidden toolbars will be visible during startup, then hidden afterwards.

Only the transition should have :root[sessionrestored] in the selector.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242144/diff/3-4/

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

https://reviewboard.mozilla.org/r/242144/#review248652

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/087157c4e7c3
Wait until the session has been restored before applying the theme transition properties. r=dao

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/087157c4e7c3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1397393 
[User impact if declined]: users may see extra visual noise while opening firefox as their theme becomes enabled
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this change just delays the theme transition until after the session is restored. the code used to determine if the session was restored was shipping in Firefox 57.
[String changes made/needed]: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8973778 [details]
Bug 1458997 - Wait until the session has been restored before applying the theme transition properties.

Fix for visual noise on startup for users with the dark theme enabled. Approved for 61.0b5.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd0e456ced8c

Comment 22

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180515100038

I verified this issue on MacOS 10.13.4 and 10.12 with FF Nightly 61.0a1(2018-05-03) and I was able to reproduce this issue. I tested with the latest Nightly 62.0a1(2018-05-14) and the issue is no longer reproducible. 

I verified this issue on Firefox 61.0b5 on MacOS 10.13.4 and 10.12 and I can confirm that the issue is fixed.