Bug 226010

Created attachment 429156 [details] Patch

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.

Created attachment 429173 [details] Patch

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.

Created attachment 429634 [details] Patch

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

<rdar://problem/78562512>

Created attachment 430235 [details] Patch

I rebased this change, removed a comment that is no longer applicable, and removed a stray debugging print statement.

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

Created attachment 430329 [details] Patch

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

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)

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

Comment on attachment 430329 [details] Patch Thanks for the reviews!

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