Closed Bug 1451745 Opened 4 years ago Closed 4 years ago

Library Bookmarks visuals not painted correctly


(Core :: DOM: Core & HTML, defect, P1)




Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified


(Reporter: csasca, Assigned: mattwoodrow)



(Keywords: regression)


(2 files)

[Affected versions]:
- 59.0.2 RC, 60.0b9, 61.0a1 (2018-04-05)

[Affected platforms]:
- macOS 10.12.6

[Steps to reproduce]:
1. Start Firefox.
2. Open Library Bookmarks via Menu-Library-Bookmarks-Show All Bookmarks or by using (Shift+Command+B).
3. Hover the mouse over the toolbar buttons and "Search Bookmarks".

[Expected result]:
The visuals are loaded when library bookmarks shows up.

[Actual result]:
The visuals are poping up while hovering with the cursor over the buttons and search box.
Check the video from the attachment.

[Regression range]:
I managed to find a regression for this issue:
Last good 2017-12-14
First bad 2017-12-15

[Additional notes]:
On macOS 10.13.3, this issue is not reproducible, but if using STR (1-2), the "Other Bookmarks" highlight is not rendered as it should, and by moving the mouse in Library window, the highlight gets rendered properly.
On 10.10 and 10.11 the issue is not reproducible.
Attached video video-1522934093.mp4
Here is the screencast with the actual result.
hmm, this looks like a paint invalidation issue...

Not sure why it's only reproducible on OSX 10.12, but if my patch regressed it would just be uncovering a pre-existing bug.
Component: Layout: View Rendering → Layout: Web Painting
Flags: needinfo?(emilio)
The way it's supposed to work is that DocumentStatesChanged schedules a paint if we've potentially changed our inactive state, and then during the next paint [1] should detect that different state and invalidate relevant themed areas.

Based on the video it looks like that's not happened, and we're only invalidating the changed pixels when something else occupying the same area gets invalidated.

Possibly something changes between DocumentStatesChanged and when we previously did the lazy update of mDocumentState? Or we're checking a different top-level window.

I can't see how either of those are possible though.

The problem is that mDocumentState is now initialized to 0 (which means active, since it doesn't include the inactive flag), even though IsTopLevelWindowInactive() is true.

We paint in this state, drawing the inactive window theme colour, but DLBI records the result of GetDocumentState() (active).

We then transition the new window to active, call DocumentStatesChanged, and leave mDocumentState unchanged (as active, correctly this time).

The next paint now thinks that no invalidation is necessary, since the result of GetDocumentState() is unchanged between paints, and we instead slowly draw the active window theme colour as other unrelated invalidations trigger us to redraw the same pixels.
Assignee: nobody → matt.woodrow
Attachment #8968370 - Flags: review?(emilio)
Component: Layout: Web Painting → DOM
Priority: -- → P1
Comment on attachment 8968370 [details] [diff] [review]

Review of attachment 8968370 [details] [diff] [review]:

Thanks for digging into this. Maybe mention in the commit message that SetContainer is the first place where IsTopLevelWindowInactive can possibly return something meaningful?
Attachment #8968370 - Flags: review?(emilio) → review+
Pushed by
Initialize mDocumentState once we get a docshell. r=emilio
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request Beta approval on this when you get a chance.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8968370 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: 1424816
[User impact if declined]: Incorrect colours on OSX chrome (inactive window colour instead of active) when opening new windows.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Manually by me.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Simple patch to just initialize the state when we create a window.
[String changes made/needed]: None
Flags: needinfo?(matt.woodrow)
Attachment #8968370 - Flags: approval-mozilla-beta?
Attachment #8968370 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Flags: qe-verify+
Comment on attachment 8968370 [details] [diff] [review]

simple patch to fix a painting issue, seems worth taking for 60 rc2
Attachment #8968370 - Flags: approval-mozilla-release?
Attachment #8968370 - Flags: approval-mozilla-release+
Attachment #8968370 - Flags: approval-mozilla-esr60+
I managed to reproduce the issue using an older version of Nightly (2018-04-05) on macOS 10.12.
I retested everything on the same platform using the latest Nihtly, esr60.0.0 - build 6 and Firefox 60.0 - build 2. The bug is not reproducing anymore.
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.