position:sticky content is not correctly offset on async scrolled overflow layers with an RTL vertical scrollbar

• Component: Compositing
• Status: RESOLVED FIXED

[Chris Lord]

This can be seen in fast/scrolling/rtl-scrollbars-sticky-overflow-scroll-2.html on WPE, where async overflow scrolling is enabled during testing. This seems very similar to bug 210820.

[Chris Lord]

I believe this does actually affect Mac, but it passes the tests either because async overflow scrolling isn't enabled during testing or because overlay scrollbars are used during testing. Patch incoming.

[Chris Lord]

Created attachment 430452 [details]
Patch

[Chris Lord]

Created attachment 430453 [details]
Patch

[Darin Adler]

Comment on attachment 430453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430453&action=review

> Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm:80
> +            constrainingRect.move(overflowScrollingNode.scrollDeltaSinceLastCommit());

Seems like we should have operator+ for these types so we could do this as expression rather than a “set to wrong value and then fix with a function call”.

[Chris Lord]

(In reply to Darin Adler from comment #4)
> Comment on attachment 430453 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430453&action=review
> 
> > Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm:80
> > +            constrainingRect.move(overflowScrollingNode.scrollDeltaSinceLastCommit());
> 
> Seems like we should have operator+ for these types so we could do this as
> expression rather than a “set to wrong value and then fix with a function
> call”.

I guess the reason there isn't is the behaviour of adding a FloatSize to a FloatRect is pretty ambiguous - in this case, we want to offset the origin, but I think adding the size to the size of the rect would be an equally reasonable thing to expect.

There is an operator+ for FloatPoint and FloatSize, so I guess this could be FloatRect(m_constraints.constrainingRectAtLastLayout().location() + overflowScrollingNode.scrollDeltaSinceLastCommit(), m_constraints.constrainingRectAtLastLayout().size()) if we wanted to keep it as a one-liner.

[Darin Adler]

(In reply to Chris Lord from comment #5)
> I think adding the size to the size of the rect would be an equally
> reasonable thing to expect.

Interesting point that I hadn’t considered.

> There is an operator+ for FloatPoint and FloatSize, so I guess this could be
> FloatRect(m_constraints.constrainingRectAtLastLayout().location() +
> overflowScrollingNode.scrollDeltaSinceLastCommit(),
> m_constraints.constrainingRectAtLastLayout().size()) if we wanted to keep it
> as a one-liner.

Doesn’t seem better.

[Simon Fraser (smfr)]

I tested the patch on macOS, it seems fine.

[Radar WebKit Bug Importer]

<rdar://problem/78826667>

[EWS]

Committed r278429 (238453@main): <https://commits.webkit.org/238453@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430453 [details].