[css-scroll-snap] Scroll snap is broken with non-horizontal writing modes

• Component: Scrolling
• Status: RESOLVED FIXED

[Martin Robinson]

The example linked above does not work and, in addition, several WPT scroll-snap tests using `writing-mode` are failing.

[Martin Robinson]

Created attachment 429156 [details]
Patch

[Martin Robinson]

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]

Created attachment 429173 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 429634 [details]
Patch

[Martin Robinson]

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

<rdar://problem/78562512>

[Martin Robinson]

Created attachment 430235 [details]
Patch

[Martin Robinson]

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

Created attachment 430329 [details]
Patch

[Martin Robinson]

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

(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 on attachment 430329 [details]
Patch

Thanks for the reviews!

[EWS]

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