1447052 - Regression: Toolbar and Tabstrip flickers white when using the a dark lightweight theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mehmet]

Attachment: Flashing.mov

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

1.) Use a the official Dark Theme or a darker Theme
2.) Create a new Window via CMD-N

Actual: The whole Toolbar+Tabstrip flickers white

Expected: No flicker.


This is a regression: Works fine with Firefox Stable.

Comment 1

[Mehmet]

I did a manual bisect and found the following Good/Bad build.

Good: 2018-02-14-22-00-40 ()

 Bad: 2018-02-14-22-48-14 ()


Maybe https://hg.mozilla.org/mozilla-central/rev/86af6b6108d8 ?

Comment 2

[Brian Grinstead [:bgrins]]

Unfortunately I can't reproduce this locally, but we did discuss in that bug potentially moving the LWT application to be an inline script instead of waiting for DOMContentLoaded which I expect could fix it: https://bugzilla.mozilla.org/show_bug.cgi?id=1434401#c12. Dão, can you reproduce this flicker?

Comment 3

[Brian Grinstead [:bgrins]]

Mehmet, do you happen to have a local build where you can reproduce this? I suspect that if you move this line `new LightweightThemeConsumer(document)` [0] to inside this condition [1] it may resolve the issue.

[0]: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1364
[1]: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1192

Comment 4

[Mehmet]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Mehmet, do you happen to have a local build where you can reproduce this? I
> suspect that if you move this line `new LightweightThemeConsumer(document)`
> [0] to inside this condition [1] it may resolve the issue.
> 
> [0]:
> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1364
> [1]:
> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1192

Hi Brian,

Unfortunately I am not a developer and have no skills to build a local version :-(

Maybe you or another dev here can build a version with your requested change and send me the link for the download, so that I can test it on my device.  

BTW: I use a Non-Retina MacBook Air. Maybe it is only reproducible on Non-Retina devices?

Thanks in advance.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Unfortunately I can't reproduce this locally, but we did discuss in that bug
> potentially moving the LWT application to be an inline script instead of
> waiting for DOMContentLoaded which I expect could fix it:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1434401#c12.

It probably makes sense to do this in onBeforeInitialXULLayout now that we have that.

> Dão, can you reproduce this flicker?

No, I don't think I've seen this.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

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

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Mehmet, do you happen to have a local build where you can reproduce this? I
> > suspect that if you move this line `new LightweightThemeConsumer(document)`
> > [0] to inside this condition [1] it may resolve the issue.
> > 
> > [0]:
> > https://searchfox.org/mozilla-central/rev/
> > 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1364
> > [1]:
> > https://searchfox.org/mozilla-central/rev/
> > 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser.js#1192
> 
> Hi Brian,
> 
> Unfortunately I am not a developer and have no skills to build a local
> version :-(
> 
> Maybe you or another dev here can build a version with your requested change
> and send me the link for the download, so that I can test it on my device.  
> 
> BTW: I use a Non-Retina MacBook Air. Maybe it is only reproducible on
> Non-Retina devices?
> 
> Thanks in advance.

If you scroll down on "Job Details" at the bottom of https://treeherder.mozilla.org/#/jobs?repo=try&revision=c38821af7e9c9f49776615762a42736ca4b8355a&selectedJob=169276685&filter-searchStr=OS%20X%20Cross%20Compiled%20opt%20build-macosx64%2Fopt%20(B) and click on target.dmg to download it, then you should be able to run it directly from the disk image. Note that you may need to right click -> Open to get around an 'unidentified developer' error (http://osxdaily.com/2012/07/27/app-cant-be-opened-because-it-is-from-an-unidentified-developer/).

Comment 8

[Mehmet]

Attachment: official_dark_theme_still_flickers_with_target_dmg.mov

(In reply to Brian Grinstead [:bgrins] from comment #7)
> If you scroll down on "Job Details" at the bottom of
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c38821af7e9c9f49776615762a42736ca4b8355a&selectedJob=1
> 69276685&filter-searchStr=OS%20X%20Cross%20Compiled%20opt%20build-
> macosx64%2Fopt%20(B) and click on target.dmg to download it, then you should
> be able to run it directly from the disk image. Note that you may need to
> right click -> Open to get around an 'unidentified developer' error
> (http://osxdaily.com/2012/07/27/app-cant-be-opened-because-it-is-from-an-
> unidentified-developer/).

Hi Brian,

okay, thanks! I was able to install and run the target.dmg

Results:

--> The Official Dark Theme still flickers in the Toolbar area.

--> The Space Fantasy Theme doesn't flicker any more.

A screencast is attached.

Comment 9

[Mehmet]

--> see comment 8 :-) Thanks.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #9)
> --> see comment 8 :-) Thanks.

Alright - can you try this one? https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9764b599fe0f1ebb3d13e818fa42fd3dcf00945&selectedJob=169303417

Comment 11

[Brian Grinstead [:bgrins]]

I've watched the screenshot and I think it's a different flicker now. I don't see any white flash anymore - it's more like the dark LWT is applied but the dark theme's darker CSS variables for the navbar aren't applied for a flash. That may not actually be a regression - I'm curious if you see that behavior in the 'good' version from Comment 1.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

Based on Comment 11, I think this patch is fixing the regression introduced by Bug 1434401

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

https://reviewboard.mozilla.org/r/229494/#review235558

Comment 14

[Mehmet]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Mehmet from comment #9)
> > --> see comment 8 :-) Thanks.
> 
> Alright - can you try this one?
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b9764b599fe0f1ebb3d13e818fa42fd3dcf00945&selectedJob=1
> 69303417

(In reply to Brian Grinstead [:bgrins] from comment #11)
> I've watched the screenshot and I think it's a different flicker now. I
> don't see any white flash anymore - it's more like the dark LWT is applied
> but the dark theme's darker CSS variables for the navbar aren't applied for
> a flash. That may not actually be a regression - I'm curious if you see that
> behavior in the 'good' version from Comment 1.


Hello Brian,

thanks for the second target.dmg file. But the Official Dark Theme still flickers. It is definitely not flickering in Firefox Stable. 

Please let me know if I can help with testing. Thanks!

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > I've watched the screenshot and I think it's a different flicker now. I
> > don't see any white flash anymore - it's more like the dark LWT is applied
> > but the dark theme's darker CSS variables for the navbar aren't applied for
> > a flash. That may not actually be a regression - I'm curious if you see that
> > behavior in the 'good' version from Comment 1.
> 
> 
> Hello Brian,
> 
> thanks for the second target.dmg file. But the Official Dark Theme still
> flickers. It is definitely not flickering in Firefox Stable. 
> 
> Please let me know if I can help with testing. Thanks!

You do see different issues before/after the patches though, right? In particular with the change there's no more white flicker, but there is a dark grey flicker.

Can you confirm if in the Good build from Comment 1 (2018-02-14-22-00-40) you see the grey flicker?

Comment 16

[Mehmet]

Hi Brian,

okay, now I am knowing what's going on.

It seems, when I noticed the flicker in Nightly two days ago, there were two separate flicker-issues:

One with Official Dark Theme and one with the Space Fantasy Theme.

I did the bisect from comment 1, but based on the Space Fantasy Theme, because I thought it is the same issue.

But the flicker with the official Dark Theme seemed to be started later. I did a further bisect in mozilla central now and found the following regression range for the official Dark Theme:

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

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

Can you please help to find the culprit change in the above mentioned range which could have cause the flicker in the official Dark Theme? Then I would open a new report for it. 

Thank you very much.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #16)
> Hi Brian,
> 
> okay, now I am knowing what's going on.
> 
> It seems, when I noticed the flicker in Nightly two days ago, there were two
> separate flicker-issues:
> 
> One with Official Dark Theme and one with the Space Fantasy Theme.
> 
> I did the bisect from comment 1, but based on the Space Fantasy Theme,
> because I thought it is the same issue.

Thanks for checking. I'm going to rename this one to reflect that it's affecting dark lightweight themes and land the fix from the first build I sent you. Then we can file a separate bug to track down what happened to the built in dark theme on 2018-03-16.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84612bb5aeb
Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad r=dao

Comment 19

[Mehmet]

Okay thanks :-) Brian/Dão: Could https://hg.mozilla.org/mozilla-central/rev/d6e5148a7f7d be the cause for the Official Dark Theme Flicker?

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #19)
> Okay thanks :-) Brian/Dão: Could
> https://hg.mozilla.org/mozilla-central/rev/d6e5148a7f7d be the cause for the
> Official Dark Theme Flicker?

Probably not. Did you get a link to a pushlog from mozregression?

Comment 21

[Mehmet]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Mehmet from comment #19)
> > Okay thanks :-) Brian/Dão: Could
> > https://hg.mozilla.org/mozilla-central/rev/d6e5148a7f7d be the cause for the
> > Official Dark Theme Flicker?
> 
> Probably not. Did you get a link to a pushlog from mozregression?

I can not use Mozregression because it doesn't work for me on Mac :-(

I did the bisect manually from https://ftp.mozilla.org/pub/firefox/nightly/2018/.

When I add the range with their timestamp to pushlog I receive the following changes:

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2018-03-16+10%3A01%3A32&enddate=2018-03-16+22%3A01%3A24

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b84612bb5aeb

Comment 23

[Brian Grinstead [:bgrins]]

Mehmet, can you verify the lightweight theme issue is fixed in Firefox Nightly? I'll request beta uplift once you confirm.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Mehmet from comment #21)
> (In reply to Brian Grinstead [:bgrins] from comment #20)
> > (In reply to Mehmet from comment #19)
> > > Okay thanks :-) Brian/Dão: Could
> > > https://hg.mozilla.org/mozilla-central/rev/d6e5148a7f7d be the cause for the
> > > Official Dark Theme Flicker?
> > 
> > Probably not. Did you get a link to a pushlog from mozregression?
> 
> I can not use Mozregression because it doesn't work for me on Mac :-(
> 
> I did the bisect manually from
> https://ftp.mozilla.org/pub/firefox/nightly/2018/.
> 
> When I add the range with their timestamp to pushlog I receive the following
> changes:
> 
> https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2018-03-
> 16+10%3A01%3A32&enddate=2018-03-16+22%3A01%3A24

Thanks - I'm guessing bug 1448043 will fix the Dark theme issue.

Comment 25

[Mehmet]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Mehmet, can you verify the lightweight theme issue is fixed in Firefox
> Nightly? I'll request beta uplift once you confirm.

Yes, it is fixed for me in the lightweight themes in Nightly 61.0a1 (2018-03-22) (64-Bit). Please feel to uplift to Beta. Thank you!

(In reply to Brian Grinstead [:bgrins] from comment #24)
> I'm guessing bug 1448043 will fix the Dark theme issue.

Great, thanks again :-)

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

Approval Request Comment
[Feature/Bug causing the regression]: 1434401
[User impact if declined]: Tab bar can flicker during startup with lightweight themes applied
[Is this code covered by automated tests?]: The LWT feature is, but the flicker itself isn't.
[Has the fix been verified in Nightly?]: Yes (Comment 25)
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: It's is moving the existing lightweight theme initialization earlier in the window startup timeline. It shouldn't change anything for users without lightweight themes applied.
[String changes made/needed]: None

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

lwt regression fix, beta60+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta uplift.

Comment 29

[Brian Grinstead [:bgrins]]

Mehmet, could you please confirm the fix on this build of beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92e9f243b0a43e5aaf259a1c5c527bcec5feeeac&selectedJob=170891418. I had to change where it gets set up since we don't have onBeforeInitialXULLayout on 60, so just want to confirm you don't see the white flicker.

Comment 30

[Mehmet]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> Mehmet, could you please confirm the fix on this build of beta:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=92e9f243b0a43e5aaf259a1c5c527bcec5feeeac&selectedJob=1
> 70891418. I had to change where it gets set up since we don't have
> onBeforeInitialXULLayout on 60, so just want to confirm you don't see the
> white flicker.

Of course, I‘ll take a look tomorrow and give you a feedback.

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: beta-lwt-consumer-ondomcontentloaded.patch

For beta uplift if we get verification this also fixes the problem. This uses DOMContentLoaded instead of onBeforeInitialXULLayout

Comment 32

[Mehmet]

(In reply to Mehmet from comment #30)
> (In reply to Brian Grinstead [:bgrins] from comment #29)
> > Mehmet, could you please confirm the fix on this build of beta:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=92e9f243b0a43e5aaf259a1c5c527bcec5feeeac&selectedJob=1
> > 70891418. I had to change where it gets set up since we don't have
> > onBeforeInitialXULLayout on 60, so just want to confirm you don't see the
> > white flicker.
> 
> Of course, I‘ll take a look tomorrow and give you a feedback.

WorksForMe in your provided build. No flashing with the offical Dark Theme or the lightweight themes.

Thanks!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/106b1066bb3f

Comment 34

[Bogdan Maris, Desktop QA]

If it's not too much to ask, Mehmet, can you also double check the fix on the latest official Beta build?
https://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/mac/en-US/

Comment 35

[Mehmet]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> If it's not too much to ask, Mehmet, can you also double check the fix on
> the latest official Beta build?
> https://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/
> mac/en-US/

Hi Bogdan,

That’s no problem, but I am on vacation ATM - no MacBook around me. I can check it earliest on coming Saturday. 

I hope it is okay for you.

Comment 36

[Bogdan Maris, Desktop QA]

(In reply to Mehmet from comment #35)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> > If it's not too much to ask, Mehmet, can you also double check the fix on
> > the latest official Beta build?
> > https://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/
> > mac/en-US/
> 
> Hi Bogdan,
> 
> That’s no problem, but I am on vacation ATM - no MacBook around me. I can
> check it earliest on coming Saturday. 
> 
> I hope it is okay for you.

That's fine thanks. BTW happy holidays! \o/

Comment 37

[Mehmet]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #36)
> (In reply to Mehmet from comment #35)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> > > If it's not too much to ask, Mehmet, can you also double check the fix on
> > > the latest official Beta build?
> > > https://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/
> > > mac/en-US/
> > 
> > Hi Bogdan,
> > 
> > That’s no problem, but I am on vacation ATM - no MacBook around me. I can
> > check it earliest on coming Saturday. 
> > 
> > I hope it is okay for you.
> 
> That's fine thanks. BTW happy holidays! \o/

Hi Bogdan,

I've tested it with your provided link to the Beta 8 Candidate build --> WorksForMe.

It is also WorkingForMe with the latest official Beta 10.

Comment 38

[Bogdan Maris, Desktop QA]

(In reply to Mehmet from comment #37)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #36)
> > (In reply to Mehmet from comment #35)
> > > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> > > > If it's not too much to ask, Mehmet, can you also double check the fix on
> > > > the latest official Beta build?
> > > > https://archive.mozilla.org/pub/firefox/candidates/60.0b8-candidates/build1/
> > > > mac/en-US/
> > > 
> > > Hi Bogdan,
> > > 
> > > That’s no problem, but I am on vacation ATM - no MacBook around me. I can
> > > check it earliest on coming Saturday. 
> > > 
> > > I hope it is okay for you.
> > 
> > That's fine thanks. BTW happy holidays! \o/
> 
> Hi Bogdan,
> 
> I've tested it with your provided link to the Beta 8 Candidate build -->
> WorksForMe.
> 
> It is also WorkingForMe with the latest official Beta 10.

Thanks again for verifying. Closing as Verified Fixed.