1471381 - All Tabs menu repaints on every scroll event

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Interestingly, this doesn't appear to be the case for other scrolling Photon menus, like the History subview inside of the Library.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I talked with Mike and he was reproducing this on OSX. I am able to reproduce it on Windows too.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 5

[Matt Woodrow (:mattwoodrow)]

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 10

[Alex Thayer [:alexical] (she/her)]

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.

Comment 11

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

Effectively we compute the diff by lopping off the rounded part
of each rect where appropriate. This gets a bit weird to think
about with very mismatched radii, but I think it works out(?)

Review commit: https://reviewboard.mozilla.org/r/256866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256866/

Comment 12

[Alex Thayer [:alexical] (she/her)]

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 13

[Matt Woodrow (:mattwoodrow)]

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 14

[Matt Woodrow (:mattwoodrow)]

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.

Comment 15

[Matt Woodrow (:mattwoodrow)]

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 16

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256866/diff/1-2/

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

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

Comment 18

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8991971 [details]
Bug 1471381 - Don't give up on rounded rects in DisplayItemClip

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256866/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6586a6e92c6a
Don't give up on rounded rects in DisplayItemClip r=mattwoodrow

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6586a6e92c6a

Comment 21

[Julien Cristau [:jcristau]]

Do you want to request uplift to 62?

Comment 22

[Alex Thayer [:alexical] (she/her)]

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.

Comment 23

[CosminB]

Attachment: 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.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00909c37c92f

Comment 26

[CosminB]

Attachment: 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.