Closed Bug 1601306 Opened 5 years ago Closed 5 years ago

Default themes are not applied correctly when they are switched from about:addons on Mac OS

Categories

(Core :: Web Painting, defect)

Desktop
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- verified
firefox73 --- verified

People

(Reporter: vvalentina, Assigned: mstange)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

[Affected Versions]:

  • Firefox Beta 72.0b2

[Affected Platforms]:

  • All Mac

[Steps to reproduce]:

  1. Go to about:addons#Themes page
  2. Enable Light theme
  3. Switch to Standard theme
  4. Observe tabs strip behavior

[Expected results]:

  • The new theme is applied and there are no display or layout issues.

[Actual results]:

  • The theme is not applied correctly on the tab strip.

[Regression]:
23:31.26 INFO: No more inbound revisions, bisection finished.
23:31.26 INFO: Last good revision: 12df7898b0eee9b11ed6189769f2a6199d93d4e1
23:31.26 INFO: First bad revision: 4d9f24928383f3da9fe40a2900a403ceb76c5983
23:31.26 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=12df7898b0eee9b11ed6189769f2a6199d93d4e1&tochange=4d9f24928383f3da9fe40a2900a403ceb76c5983

It looks like bug 1592739 has caused this behavior.

[Notes]

  • Issue observed better while hovering over the tab strip.

@mstange, could you please take a look at this?

Flags: needinfo?(mstange)

Valentina, do you have a screenshot by any chance ? I can't reproduce this on my side.

Also, do you have WebRender enabled ?

Thanks!

Flags: needinfo?(valentina.peleskei)

I can reproduce this, but only on about:addons, not from the Customize... tab.

Flags: needinfo?(mstange)
Flags: needinfo?(valentina.peleskei)

The PaintedLayer for the chrome contains opaque pixels instead of transparent pixels in the bad paints. That's even though the entire layer is invalidated and there's no SolidColor item in the display list after the switch from light -> default. I'm not sure where the opaque pixels come from. It's possible that we're forgetting to clear previous layer content. The PaintedLayer itself is not marked as opaque. But sometimes its parent layer (the ClientLayerManager's root container layer) is marked as opaque.

This bug does not happen with WebRender enabled.

Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attached file display-dump.log (obsolete) (deleted) —
Attached file display-dump.log (deleted) —
Attachment #9113570 - Attachment is obsolete: true
Attachment #9113572 - Attachment mime type: application/octet-stream → text/plain

This has to do with mIsOpaque on the window's root display list. Its value changes between true and false somewhat randomly, as far as I can tell. If mIsOpaque is false in the paint where we change from the light theme to the default theme, we get broken rendering. If it happens to be true, we get correct rendering.

This is a retained display list bug. Sometimes mIsOpaque=true from a previous frame can stick around, and then the bad value circles back through layers. (displayList->IsOpaque() causes us to set CONTENT_OPAQUE on a layer, and then the opaque layer region results in a call to displaylist->SetIsOpaque()).
This bug does not happen if layout.display-list.retain.chrome is set to false.

These values start out false in the other two ways display lists can be build:

  • Without retained display lists, a new nsDisplayList object is created which starts out with these values being false.
  • When partial update succeeds (or at least gets far enough to run PreProcessDisplayList), RestoreState() is called on the root display list which sets these values to false.

But when partial update fails, we have an old nsDisplayList object and we were not clearing those values.

Regressed by: 1413546
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/17464983f090 When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Should this be uplifted to 72?

Flags: needinfo?(mstange)

Reproduced the initial issue using an old Nightly build: 20191204215924 and Beta 72.0b2.
Verified - fixed on latest Nightly 73.0a1 (2019-12-05) (Build id: 20191205041108) on Mac OS 10.14.

Comment on attachment 9113597 [details]
Bug 1601306 - When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

Beta/Release Uplift Approval Request

  • User impact if declined: UI glitches when switching themes
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: see comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The fix is very simple.
  • String changes made/needed:
Flags: needinfo?(mstange)
Attachment #9113597 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]

Comment on attachment 9113597 [details]
Bug 1601306 - When partial update fails, make sure that mIsOpaque and mForceTransparentSurface on the root display list are false. r=tnikkel

macos regression fix, verified in nightly, approved for 72.0b5

Attachment #9113597 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified - fixed on latest Beta 72.0b5 (Build id: 20191209175918) on Mac OS 10.14.

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: