Closed Bug 1471381 Opened Last year Closed Last year

All Tabs menu repaints on every scroll event

Categories

(Core :: Web Painting, defect, P3)

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: mconley, Assigned: dthayer)

References

Details

(Keywords: regression, Whiteboard: [fxperf:p1])

Attachments

(3 files)

STR:

1. Have enough tabs such that the new Photon All Tabs menu can be scrolled
2. Enable browser UI paint flashing by setting nglayout.debug.paint_flashing_chrome to true
3. Open the All Tabs menu, and scroll

ER:

The menu should repaint the areas the are being presented to the user for the first time. The rest of the scrolled area should not change colour.

AR:

The whole thing changes colour each time, meaning that we're repainting the whole thing.
Interestingly, this doesn't appear to be the case for other scrolling Photon menus, like the History subview inside of the Library.
It _is_ however, true for the History button (in the customize mode palette by default), which is scrollable in its "main" view.

So this appears to be a more general problem. Moving it to the Firefox :: Menus component for now.
Component: Tabbed Browser → Menus
Priority: -- → P3
I talked with Mike and he was reproducing this on OSX. I am able to reproduce it on Windows too.
Interestingly, scrolling inside a subview (a submenu within a panel) scrolls nicely. Even stranger, for menus where I can reproduce the overpainting, if I enter a subview in that menu, and then go back up a level to the original overpainting menu, the bug disappears.

Demonstration: https://www.screencast.com/t/Mxk3yotyTY

I suspect this might actually be a graphics issue. :/ Hey mattwoodrow, do you know who could help us figure out why we're overpainting here? There's nothing obvious to us in our CSS or JS that would cause this.
Flags: needinfo?(matt.woodrow)
I did some debugging, quite the rabbit hole.

Firstly, we don't get OMTC for popups (on OSX at least), and no APZ either. Enabling OMTC seems to at least work, APZ doesn't respond to mouse scroll, and crashes on keyboard scrolls.

The reason we're invalidating is because there's a rounded rect clip (on a box) outside the scroll frame. 

With APZ, we're not being smart enough to detect that the clip and scrolled content move separately, so they're all in the same layer and we detect that the content has moved relative to the clip. For rectangular clips we're smart enough to only invalidate areas actually affected by the clip (the difference of the new/old clipped areas), but we give up when we get rounded corners [1]. Bug 1336516 would help here.

Switching to a sub-panel and then back again seems to have the effect of removing the rounded rect clip. No idea why, but once it's gone, then scrolling works just fine.


[1] https://searchfox.org/mozilla-central/source/layout/painting/DisplayItemClip.cpp#376
Flags: needinfo?(matt.woodrow)
This seems to be due to this rule: https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/toolkit/themes/osx/global/popup.css#47

I believe there was some effort to eliminate rounded corners on our doorhangers in bug 1381554. Maybe we should try that again.
Depends on: 1381554
Whiteboard: [fxperf] → [fxperf:p2]
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #6)
> This seems to be due to this rule:
> https://searchfox.org/mozilla-central/rev/
> 28daa2806c89684b3dfa4f0b551db1d099dda7c2/toolkit/themes/osx/global/popup.
> css#47

Jared said he could reproduce this on Windows where we don't use border-radius.
Hum. Then perhaps I'm mistaken - although when I was debugging this with mattwoodrow last week, we ended up centering on this as the cause...

On my Windows box, I'm actually not able to reproduce the overpainting. jaws, can you re-confirm that you did in fact see overpainting going on in the tabs drop down - overpainting similar to the video I posted in comment 4?
Flags: needinfo?(jaws)
I can't reproduce this anymore on Windows. I suspect that I had a tab that was refreshing which caused the layout to invalidate. See video (sorry, requires Flash) of me attempting to reproduce this again on Windows, https://www.screencast.com/t/TUWYwAffKF
Flags: needinfo?(jaws)
OS: Unspecified → Mac OS X
Taking a look at this. It looks to me that while I see why the rounded corners logic is a bit complicated, it's totally doable.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Whiteboard: [fxperf:p2] → [fxperf:p1]
Put up a scary helper function for diffing rounded rects. The paint flashing looks good for this, but if anyone has any suggestions on deeper ways to test this let me know.
Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

https://reviewboard.mozilla.org/r/256866/#review263906

::: layout/painting/DisplayItemClip.cpp:363
(Diff revision 1)
> +                                const DisplayItemClip::RoundedRect& aR2,
> +                                const nsRect& aBounds,
> +                                const nsRect& aOtherBounds,
> +                                nsRegion* aOut)
> +{
> +  nscoord r1Bottom = aR1.mRect.y + aR1.mRect.height;

We have X() and XMost() accessors on BaseRect, can we just use those directly instead of computing temporaries upfront?

::: layout/painting/DisplayItemClip.cpp:374
(Diff revision 1)
> +  nscoord r2Right = aR2.mRect.x + aR2.mRect.width;
> +  nscoord r2Left = aR2.mRect.x;
> +
> +  // If the two rectangles are totally disjoint, just add them both - otherwise we'd
> +  // end up adding one big enclosing rect
> +  if (r1Top >= r2Bottom || r2Top >= r1Bottom ||

Can we just do if (!aR1.mRect.Intersects(aR2.mRect)) for this condition?

::: layout/painting/DisplayItemClip.cpp:381
(Diff revision 1)
> +    aOut->Or(*aOut, aR1.mRect.Intersect(aBounds));
> +    aOut->Or(*aOut, aR2.mRect.Intersect(aOtherBounds));
> +    return;
> +  }
> +
> +  nscoord lowestBottom = std::max(r1Bottom, r2Bottom);

How about computing the bounds rect using BaseRect::Union, and then using X/XMost etc on that?

::: layout/painting/DisplayItemClip.cpp:386
(Diff revision 1)
> +  nscoord lowestBottom = std::max(r1Bottom, r2Bottom);
> +  nscoord highestTop = std::min(r1Top, r2Top);
> +  nscoord maxRight = std::max(r1Right, r2Right);
> +  nscoord minLeft = std::min(r1Left, r2Left);
> +
> +  nscoord highestAdjustedBottom =

Can you please add a decent sized inline comment here explaining the logic of what we're doing?

I think it works, but took me a while to get my head around :)

::: layout/painting/DisplayItemClip.cpp:410
(Diff revision 1)
> +                      std::max(r2Left + aR2.mRadii[eCornerTopLeftX],
> +                               r2Left + aR2.mRadii[eCornerBottomLeftX])));
> +
> +  nsRegion r;
> +  // First, or with the Y delta rects, wide along the X axis
> +  if (r1Top != r2Top) {

Won't this not run if we just changed the radii without moving the base rectangle?

I think we should just check for changed radii, and invalidate the whole thing if so, and then we simplify this to code to only handle the move case.

It looks like we'll always invalidate the whole area already if the radii changes (nsStyleBorder::CalcDifference issues a nsChangeHint_RepaintFrame for a radius change, which invalidate the whole frame subtree), so I don't think the extra complexity helps us here.
Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

https://reviewboard.mozilla.org/r/256866/#review263906

This is pretty awesome, thanks! Just a few simplification comments.
As a side note, I'd been thinking about how to solve this too.

My idea:

* Compute the 4 corner rectangles for each RoundedRect (the smallest rectangle that bounds the arc, width and height pulled from the radii, and one corner defined by the bounding rect).
* Diff each of those between RoundedRects, if not the same, add both to the invalidation region.
* Compute the 'definitely within clip' region for each RoundedRect, but taking the bounds rect and Sub()ing the 4 corner rectangles.
* Add the Xor() of the two 'definitely within clip' regions to the invalidation region.

Probably a lot more time spent in region manipulation code that way.
Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

https://reviewboard.mozilla.org/r/256866/#review264192
Attachment #8991971 - Flags: review?(matt.woodrow) → review+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6586a6e92c6a
Don't give up on rounded rects in DisplayItemClip r=mattwoodrow
Component: Menus → Layout: Web Painting
Product: Firefox → Core
https://hg.mozilla.org/mozilla-central/rev/6586a6e92c6a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Do you want to request uplift to 62?
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(dothayer)
Keywords: regression
Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1446101
[User impact if declined]: All tabs photon panel and standalone history photon panel will jank when scrolled on OSX.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[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?]: I would say it's fairly low risk.
[Why is the change risky/not risky?]: Worst case is us not painting certain areas of moving/scrolling rounded rectangles.
[String changes made/needed]: None.
Flags: needinfo?(dothayer)
Attachment #8991971 - Flags: approval-mozilla-beta?
Attached image Bug1471381.gif
I was able to reproduce this issue on Firefox 62.0b9(20180713213322) under Win 7 64-bit and Mac OS X 10.13.3.

I was not able to reproduce this issue on Firefox 62.0b9(20180713213322) under Win 10 64-bit.

This issue is verified as fixed on Firefox 63.0a1 (20180719100142) under Win 7 64-bit, Win 10 64-bit and Mac OS X 10.13.3.

Please see the attached video.
Status: RESOLVED → VERIFIED
Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

Fix for recent regression, verified in nightly. 
Let's take this for beta 11.
Attachment #8991971 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image BetaFix.gif
This issue is verified as fixed on Firefox 62.0b11(20180723144101) under Win 7 64-bit and Mac OS X 10.13.3.
You need to log in before you can comment on or make changes to this bug.