WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(25.86 KB, patch)
2021-05-20 07:47 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Patch
(25.30 KB, patch)
2021-05-25 00:27 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Patch
(25.15 KB, patch)
2021-06-01 00:28 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Patch
(24.83 KB, patch)
2021-06-02 02:39 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Martin Robinson
Comment 1
2021-05-20 04:53:27 PDT
Created
attachment 429156
[details]
Patch
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
Created
attachment 429173
[details]
Patch
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
Created
attachment 429634
[details]
Patch
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
<
rdar://problem/78562512
>
Martin Robinson
Comment 8
2021-06-01 00:28:27 PDT
Created
attachment 430235
[details]
Patch
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
Created
attachment 430329
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug