1562184 - Windows 7 glass shading in non-client area randomly flickers based on mouse movement

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Jim Mathies [:jimm]]

Attachment: glass shade example

Somewhere in layout we call into widget to set the height of the glass area for the main browser window. It looks like there's a bug here that's changing the height randomly based on mouse movements. Hence the shade on the side of the window chrome flickers on and off as you move your mouse around content.

Comment 1

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:

Comment 2

[Kate Hudson :k88hudson]

Can you explain this a little better / record a video so we can help find a regression window?

Comment 3

[M8R-p7]

Attachment: bug1562184.webm

Here's a video of the effect - the first transition is from an inactive window to an active one, which is correct. Later changes are incorrect. Making the mouse change cursors seems a reliable way to cause it for me, but it doesn't seem to be a requirement.

This looks like the range from the builds available on FTP:
Good: 20190603160429
Bad: 20190604034844

Comment 4

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0ca9a03071b76cb0aca81b83ed451dedd2b7c1f5&tochange=87409f291fa6a003f00ae782528bcd4734eeaee0

Regressed by:
87409f291fa6a003f00ae782528bcd4734eeaee0 Miko Mynttinen — Bug 1413546 - Enable retained display lists for parent process r=mattwoodrow

Comment 5

[Timothy Nikkel (:tnikkel)]

A regression range with layout.display-list.retain.chrome set to true might be interesting (if it regressed that is). I'll get to it if no one gets to it before me.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

According to bug 1563297 removing this CSS rule "fixes" the issue.

Comment 8

[Timothy Nikkel (:tnikkel)]

Hmm, doesn't seem to be a regression with layout.display-list.retain.chrome enabled. Bug 1554373 was supposed to handle exactly this case.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

According to bug 1563297 removing this CSS rule "fixes" the issue.

Bug 633282 introduced this to fix the moving glass border issue back in the day.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Dão Gottwald [::dao] from comment #9)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

According to bug 1563297 removing this CSS rule "fixes" the issue.

Bug 633282 introduced this to fix the moving glass border issue back in the
day.

Yeah, but removing that rule probably means that instead of moving around the glass effect is just always too low (instead of moving too low/too high).

Comment 11

[Sean Voisen (:svoisen)]

This bug can use an owner since we're tracking for 69.

tnikkel: Any chance might be able to continue look into this? And is this better moved into Web Painting?

Comment 12

[Timothy Nikkel (:tnikkel)]

Yeah, I am looking into this.

Comment 14

[Timothy Nikkel (:tnikkel)]

This is pretty silly. We intersect the exclude glass region with the window opaque region to set the glass area on the window. The exclude glass region is a WeakFrameRegion (ie it can handle retained display list partial updates), the window opaque region is a plain nsRegion, so if we do a partial build it can get set to empty because it gets cleared every partial update.

The window opaque region comes from accumulating the GetOpaqueRegion from display items. Luckily GetOpaqueRegion is always a rect for painting for perf reasons (it's only a region for hit testing some of the time). This still could mean that more than one display item for the same frame are contributing different rects for the opaque region would could change the final result (WeakFrameRegion can only handle one rect per frame for perf reasons).

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1562184. Convert window opaque region to a WeakFrameRegion. r?mattwoodrow

For retained display lists it needs to be a weak frame region to areas get removed for modified frames and the rest stick around.

Each display item can contribute an opaque region but WeakFrameRegion can only track one rect per frame. It turns out that we only return rects from GetOpaqueRegion except in one hit testing case.

This still means more than one display item per frame could be contributing to the opaque region, we would miss the second and further rects in that case.

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1562184. Have a regular nsRegion for the window opaque region for non-retained cases. r?mattwoodrow

Not sure if we need this, but the window dragging regions have it, so in case we need to turn off retained display lists we have this and there will be no difference to before the patches in this bug.

Depends on D38589

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13fcc2ce3c28
Convert window opaque region to a WeakFrameRegion. r=mattwoodrow

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a8b3e0a54cc
Have a regular nsRegion for the window opaque region for non-retained cases. r=mattwoodrow

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/13fcc2ce3c28
https://hg.mozilla.org/mozilla-central/rev/8a8b3e0a54cc

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9079209 [details]
Bug 1562184. Have a regular nsRegion for the window opaque region for non-retained cases. r?mattwoodrow

Beta/Release Uplift Approval Request

• User impact if declined: Windows 7 glass effect on our window jumps around and is annoying
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): it slightly changes the way the opaque region is calculated but no problems have been reported on nightly
• String changes made/needed: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079207 [details]
Bug 1562184. Convert window opaque region to a WeakFrameRegion. r?mattwoodrow

Fixes a visual glitch on Windows 7 systems caused by enabling RDL in the parent process. No regressions reported since this landed in Nightly. Approved for 69.0b10.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2c3491f0154f
https://hg.mozilla.org/releases/mozilla-beta/rev/30a3d6b7d0dc

Comment 24

[Cristian Fogel [:cfogel]]

Confirmed issue with 69.0b5.
Fix verifiied with 69.0b10, 70.0a1 (2019-08-04).