Closed Bug 1562184 Opened 5 months ago Closed 4 months ago

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

Categories

(Core :: Web Painting, defect, P2)

Desktop
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- unaffected
firefox69 + verified
firefox70 --- verified

People

(Reporter: jimm, Assigned: tnikkel)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

Attached image 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.

[Tracking Requested - why for this release]:

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

Flags: needinfo?(jmathies)
See Also: → 1563297
Attached video 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

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

Regressed by: 1413546

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.

Duplicate of this bug: 1563297

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

Has STR: --- → yes
Flags: needinfo?(jmathies)

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

(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.

(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).

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?

Flags: needinfo?(tnikkel)

Yeah, I am looking into this.

Component: Layout → Web Painting
Flags: needinfo?(tnikkel)
Assignee: nobody → tnikkel
Duplicate of this bug: 1565138
Priority: -- → P2

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).

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.

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

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13fcc2ce3c28
Convert window opaque region to a WeakFrameRegion. r=mattwoodrow
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a8b3e0a54cc
Have a regular nsRegion for the window opaque region for non-retained cases. r=mattwoodrow
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Flags: needinfo?(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
Flags: needinfo?(tnikkel)
Attachment #9079209 - Flags: approval-mozilla-beta?
Attachment #9079207 - Flags: approval-mozilla-beta?

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.

Attachment #9079207 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9079209 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.