RESOLVED FIXED 226010
[css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
https://bugs.webkit.org/show_bug.cgi?id=226010
Summary [css-scroll-snap] Scroll snap is broken with non-horizontal writing modes
Martin Robinson
Reported 2021-05-20 04:20:29 PDT
The example linked above does not work and, in addition, several WPT scroll-snap tests using `writing-mode` are failing.
Attachments
Patch (22.73 KB, patch)
2021-05-20 04:53 PDT, Martin Robinson
no flags
Patch (25.86 KB, patch)
2021-05-20 07:47 PDT, Martin Robinson
no flags
Patch (25.30 KB, patch)
2021-05-25 00:27 PDT, Martin Robinson
no flags
Patch (25.15 KB, patch)
2021-06-01 00:28 PDT, Martin Robinson
no flags
Patch (24.83 KB, patch)
2021-06-02 02:39 PDT, Martin Robinson
no flags
Martin Robinson
Comment 1 2021-05-20 04:53:27 PDT
Martin Robinson
Comment 2 2021-05-20 05:33:39 PDT
Upon reading the spec a bit more, it seems the example listed above should only work if the writing-mode declaration is applied to the scroll container.
Martin Robinson
Comment 3 2021-05-20 07:47:02 PDT
Simon Fraser (smfr)
Comment 4 2021-05-24 10:40:03 PDT
Comment on attachment 429173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429173&action=review > Source/WebCore/page/FrameView.cpp:957 > + WritingMode writingMode = (bodyRenderer ? bodyRenderer->style() : rootRenderer->style()).writingMode(); > + TextDirection direction = (bodyRenderer ? bodyRenderer->style() : rootRenderer->style()).direction(); resolveForDocument() already does this computation I think.
Martin Robinson
Comment 5 2021-05-25 00:27:38 PDT
Martin Robinson
Comment 6 2021-05-25 00:30:29 PDT
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 429173 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429173&action=review > > > Source/WebCore/page/FrameView.cpp:957 > > + WritingMode writingMode = (bodyRenderer ? bodyRenderer->style() : rootRenderer->style()).writingMode(); > > + TextDirection direction = (bodyRenderer ? bodyRenderer->style() : rootRenderer->style()).direction(); > > resolveForDocument() already does this computation I think. It seems that it does. I'm not sure exactly why I thought I was observing a difference here, but I removed this bogus workaround and the tests still pass so I was probably just confused. Thanks for taking a look at this. I've uploaded the new version of the change.
Radar WebKit Bug Importer
Comment 7 2021-05-27 04:21:15 PDT
Martin Robinson
Comment 8 2021-06-01 00:28:27 PDT
Martin Robinson
Comment 9 2021-06-01 00:33:23 PDT
I rebased this change, removed a comment that is no longer applicable, and removed a stray debugging print statement.
Frédéric Wang (:fredw)
Comment 10 2021-06-01 06:50:32 PDT
Comment on attachment 430235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430235&action=review This looks good to me overall, but I have a few comments inline... > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:224 > + bool scrollerHasVerticalWritingMode = isVerticalWritingMode(writingMode); I'm not really clear how CSS direction and writing-mode properties interact together, and with scrop-snap-axis. Can you please explain, or do you have a link to the relevant part of the specs? > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:259 > + ScrollSnapAxisAlignType yAlign = !scrollerHasVerticalWritingMode ? alignment.blockAlign : alignment.inlineAlign; nit: I would do xAlign = scrollerHasVerticalWritingMode ? alignment.blockAlign : alignment.inlineAlign yAlign = scrollerHasVerticalWritingMode ? alignment.inlineAlign : alignment.blockAlign > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:277 > + auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.maxY(), yAlign, false) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.maxY(), yAlign, false); For example, here I wonder what happens with an exotic setup like "writing-mode: vertical-lr; and direction: rtl;". Shouldn't we pass scrollerIsRTL too? Is it covered by existing WPT tests? > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:290 > + This seems an unrelated change that does not affect tests. Can you please explain it? (probably it can be moved into a separate patch).
Martin Robinson
Comment 11 2021-06-02 02:39:48 PDT
Martin Robinson
Comment 12 2021-06-02 03:03:47 PDT
(In reply to Frédéric Wang (:fredw) from comment #10) > Comment on attachment 430235 [details] > Patch > Thanks for the review! > View in context: > https://bugs.webkit.org/attachment.cgi?id=430235&action=review > > This looks good to me overall, but I have a few comments inline... > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:224 > > + bool scrollerHasVerticalWritingMode = isVerticalWritingMode(writingMode); > > I'm not really clear how CSS direction and writing-mode properties interact > together, and with scrop-snap-axis. Can you please explain, or do you have a > link to the relevant part of the specs? This is a really good point. I've rewritten this a bit so that it's more complete and understandable. In addition, I've added a comment explaining everything. > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:259 > > + ScrollSnapAxisAlignType yAlign = !scrollerHasVerticalWritingMode ? alignment.blockAlign : alignment.inlineAlign; > > nit: I would do Fixed. > xAlign = scrollerHasVerticalWritingMode ? alignment.blockAlign : > alignment.inlineAlign > yAlign = scrollerHasVerticalWritingMode ? alignment.inlineAlign : > alignment.blockAlign > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:277 > > + auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.maxY(), yAlign, false) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.maxY(), yAlign, false); > > For example, here I wonder what happens with an exotic setup like > "writing-mode: vertical-lr; and direction: rtl;". Shouldn't we pass > scrollerIsRTL too? Is it covered by existing WPT tests? I've fixed this. This is covered by an existing WPT reference test, but it combines a bunch of test cases. The test progresses, but overall the test still fails. > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:290 > > + > > This seems an unrelated change that does not affect tests. Can you please > explain it? (probably it can be moved into a separate patch). Okay. I've removed this.
Frédéric Wang (:fredw)
Comment 13 2021-06-02 03:36:50 PDT
Comment on attachment 430329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430329&action=review > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:241 > + } Maybe this whole block would be easier to read with just a switch (scrollSnapType.axis)
Martin Robinson
Comment 14 2021-06-02 03:46:50 PDT
(In reply to Frédéric Wang (:fredw) from comment #13) > Comment on attachment 430329 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430329&action=review > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:241 > > + } > > Maybe this whole block would be easier to read with just a switch > (scrollSnapType.axis) I did try this out at first (and again just now). The result is quite a bit longer and, in my opinion, a bit harder to read.
Martin Robinson
Comment 15 2021-06-02 05:19:22 PDT
Comment on attachment 430329 [details] Patch Thanks for the reviews!
EWS
Comment 16 2021-06-02 05:37:06 PDT
Committed r278350 (238382@main): <https://commits.webkit.org/238382@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430329 [details].
Note You need to log in before you can comment on or make changes to this bug.