Bug 193671 - Scroll Snap broken when using RTL layout
Summary: Scroll Snap broken when using RTL layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Safari 12
Hardware: iPhone / iPad iOS 12
: P1 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: HTML5, InRadar
: 202377 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-22 08:43 PST by Graham
Modified: 2020-09-08 12:46 PDT (History)
20 users (show)

See Also:


Attachments
WIP (22.78 KB, patch)
2020-02-16 21:28 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
WIP (22.12 KB, patch)
2020-07-09 03:03 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (51.19 KB, patch)
2020-07-25 22:23 PDT, Simon Fraser (smfr)
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Graham 2019-01-22 08:43:49 PST
Hi,

The scroll-snap behavior is broken when the HTML dir attribute is set to "rtl"

It starts on the right-most element as expected, but despite the child elements, there are only three scroll-snap points. 
One at the right-most position of the container, 
another at the almost-left-most position (maybe 30px offset from the left-most position) of the container,
and a final one at the left-most position of the container.
So the scroll-snap behavior skips over the middle elements which is not intended.

A example of the issue can be found here: https://codepen.io/theGrahamScanlon/full/KJKrJa
Please view the example using an iOS device to successfully reproduce the issue.

The device I'm running:
iPhone X (Model MQA62LL/A)
Version 12.1.2 (16C101)


This effects the three major browsers on iOS: Safari, Chrome and Firefox.


Thanks,
Graham

Please let me know if you have any questions.
Comment 1 Radar WebKit Bug Importer 2019-01-22 13:22:42 PST
<rdar://problem/47457471>
Comment 2 Wenson Hsieh 2019-10-01 07:25:50 PDT
*** Bug 202377 has been marked as a duplicate of this bug. ***
Comment 3 Elliott Sprehn 2019-10-01 11:12:10 PDT
This is impacting us at Airbnb and we've had to disable scroll snap for our users in Safari. @simon.fraser Would it be possible to get this prioritized? :)
Comment 4 Simon Fraser (smfr) 2020-02-16 21:28:44 PST
Created attachment 390899 [details]
WIP
Comment 5 spacest 2020-04-14 08:50:57 PDT
Just encountered this shocking bug. Please fix it, thanks.
Comment 6 Charlie Croom 2020-05-26 11:47:47 PDT
Wanted to add in that Twitter is exploring replacing a lot of our currently JS-heavy code with native web solutions including scrollSnap. However, the lack of support internationally means that scrollSnap is effectively not implemented in Safari, so we will also have to weigh disabling it or polyfillying.

Would be great to get some of these really critical bugs fixed! Thanks!
Comment 7 Brent Fulgham 2020-05-26 20:07:49 PDT
(In reply to Charlie Croom from comment #6)
> Wanted to add in that Twitter is exploring replacing a lot of our currently
> JS-heavy code with native web solutions including scrollSnap. However, the
> lack of support internationally means that scrollSnap is effectively not
> implemented in Safari, so we will also have to weigh disabling it or
> polyfillying.
> 
> Would be great to get some of these really critical bugs fixed! Thanks!

I would love to get Scroll Snap into proper shape. I’ll see if Wenson and I can take another look soon.

Thank you for reaching out to help me gauge priority/interest.
Comment 8 Antoine Quint 2020-07-09 03:03:15 PDT
Created attachment 403854 [details]
WIP
Comment 9 Simon Fraser (smfr) 2020-07-25 22:23:23 PDT
Created attachment 405241 [details]
Patch
Comment 10 Wenson Hsieh 2020-07-26 16:58:08 PDT
Comment on attachment 405241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405241&action=review

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:59
> +static LayoutUnit computeScrollSnapAlignOffset(LayoutUnit minLocation, LayoutUnit maxLocation, ScrollSnapAxisAlignType alignment, bool isRTL)

Nit - it seems a bit confusing that isRTL is false when computing offsets in the y-axis, even though we could be in RTL. What do you think about `isFlipped`, or `startsAtMaxLocation`?
Comment 11 Simon Fraser (smfr) 2020-07-26 19:04:22 PDT
https://trac.webkit.org/changeset/264908/webkit
Comment 12 spacest 2020-09-08 12:42:32 PDT
Thanks for fixing it in Safari 14. Now RTL scroll snap doesn't work when embedded inside a RTL scroll snap item, unlike Chrome and FF. Please fix that too. Thanks
Comment 13 Antoine Quint 2020-09-08 12:46:40 PDT
Could you file a dedicated bug for this specific issue, ideally with a test? This would help towards writing a fix. Thank you.