Closed Bug 1447052 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- verified
firefox61 --- verified

People

(Reporter: mehmet.sahin, Assigned: bgrins)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached video 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.
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 ?
Flags: needinfo?(bgrinstead)
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?
Blocks: 1434401
Flags: needinfo?(bgrinstead) → needinfo?(dao+bmo)
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
Flags: needinfo?(mehmet.sahin)
Priority: -- → P1
(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.
Flags: needinfo?(mehmet.sahin) → needinfo?(bgrinstead)
(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.
Flags: needinfo?(dao+bmo)
(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/).
Flags: needinfo?(bgrinstead) → needinfo?(mehmet.sahin)
(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.
Flags: needinfo?(mehmet.sahin)
--> see comment 8 :-) Thanks.
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead) → needinfo?(mehmet.sahin)
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 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
Attachment #8960740 - Flags: review?(dao+bmo)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

https://reviewboard.mozilla.org/r/229494/#review235558
Attachment #8960740 - Flags: review?(dao+bmo) → review+
(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!
Flags: needinfo?(mehmet.sahin) → needinfo?(bgrinstead)
(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?
Flags: needinfo?(bgrinstead) → needinfo?(mehmet.sahin)
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.
Flags: needinfo?(mehmet.sahin) → needinfo?(bgrinstead)
(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.
Summary: Regression: Toolbar and Tabstrip flickers white when using the official Dark Theme or a darker Theme → Regression: Toolbar and Tabstrip flickers white when using the a dark lightweight theme
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b84612bb5aeb
Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad r=dao
Okay thanks :-) Brian/Dão: Could https://hg.mozilla.org/mozilla-central/rev/d6e5148a7f7d be the cause for the Official Dark Theme Flicker?
(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?
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/b84612bb5aeb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1448017
Blocks: 1448043
Mehmet, can you verify the lightweight theme issue is fixed in Firefox Nightly? I'll request beta uplift once you confirm.
Flags: needinfo?(mehmet.sahin)
(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.
Flags: needinfo?(bgrinstead)
Blocks: 1448059
(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 :-)
Flags: needinfo?(mehmet.sahin)
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
Attachment #8960740 - Flags: approval-mozilla-beta?
Comment on attachment 8960740 [details]
Bug 1447052 - Set up LightweightThemeConsumer in onBeforeInitialXULLayout instead of onLoad

lwt regression fix, beta60+
Attachment #8960740 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta uplift.
Flags: needinfo?(bgrinstead)
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.
Flags: needinfo?(bgrinstead) → needinfo?(mehmet.sahin)
(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.
For beta uplift if we get verification this also fixes the problem. This uses DOMContentLoaded instead of onBeforeInitialXULLayout
Attachment #8963426 - Flags: review?(dao+bmo)
Attachment #8963426 - Flags: review?(dao+bmo) → review+
(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!
Flags: needinfo?(mehmet.sahin)
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/
Flags: needinfo?(mehmet.sahin)
(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.
(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/
(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.
Flags: needinfo?(mehmet.sahin)
(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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1447100
See Also: → 1485629
You need to log in before you can comment on or make changes to this bug.