Summary: | Scroll Snap broken when using RTL layout | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Graham <webkit> | ||||||||
Component: | Scrolling | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, callie.riggins, charlie.croom, cmarcelo, esprehn, ews-watchlist, fred.wang, graouts, graouts, jamesr, luiz, miguel, rado, simon.fraser, tonikitoo, webkit-bug-importer, webkit, webkit, wenson_hsieh, zalan | ||||||||
Priority: | P1 | Keywords: | HTML5, InRadar | ||||||||
Version: | Safari 12 | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | iOS 12 | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=207826 | ||||||||||
Attachments: |
|
Description
Graham
2019-01-22 08:43:49 PST
*** Bug 202377 has been marked as a duplicate of this bug. *** 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? :) Created attachment 390899 [details]
WIP
Just encountered this shocking bug. Please fix it, thanks. 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! (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. Created attachment 403854 [details]
WIP
Created attachment 405241 [details]
Patch
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`? 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 Could you file a dedicated bug for this specific issue, ideally with a test? This would help towards writing a fix. Thank you. |