Bug 226327 - position:sticky content is not correctly offset on async scrolled overflow layers with an RTL vertical scrollbar
Summary: position:sticky content is not correctly offset on async scrolled overflow la...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 224596
  Show dependency treegraph
 
Reported: 2021-05-27 05:55 PDT by Chris Lord
Modified: 2021-06-03 16:39 PDT (History)
14 users (show)

See Also:


Attachments
Patch (3.13 KB, patch)
2021-06-03 02:06 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2021-06-03 02:10 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-05-27 05:55:46 PDT
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.
Comment 1 Chris Lord 2021-06-03 02:02:31 PDT
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.
Comment 2 Chris Lord 2021-06-03 02:06:55 PDT
Created attachment 430452 [details]
Patch
Comment 3 Chris Lord 2021-06-03 02:10:56 PDT
Created attachment 430453 [details]
Patch
Comment 4 Darin Adler 2021-06-03 07:37:33 PDT
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”.
Comment 5 Chris Lord 2021-06-03 07:43:20 PDT
(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.
Comment 6 Darin Adler 2021-06-03 09:18:50 PDT
(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.
Comment 7 Simon Fraser (smfr) 2021-06-03 11:08:16 PDT
I tested the patch on macOS, it seems fine.
Comment 8 Radar WebKit Bug Importer 2021-06-03 11:09:16 PDT
<rdar://problem/78826667>
Comment 9 EWS 2021-06-03 16:39:41 PDT
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].