Summary: | Scroll-snap points should be triggered during programmatic scroll | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Martin Robinson <mrobinson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, changseok, cmarcelo, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, jonlee, kangil.han, kondapallykalyan, luiz, m.goleb+bugzilla, mifenton, mmaxfield, mrobinson, pdr, simon.fraser, tonikitoo, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=227246 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 218115, 221302 | ||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2015-05-22 17:20:27 PDT
This is currently under discussion in the css working group. The current thought is that we should not invoke scroll snap points for programmatic scrolls, but we should add a 'snap' element to the ScrollOptions dictionary. Created attachment 417363 [details]
Patch
Created attachment 417365 [details]
Patch
Created attachment 417371 [details]
Patch
Comment on attachment 417371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417371&action=review > Source/WebCore/dom/Element.cpp:991 > + FloatPoint originalPosition = FloatPoint( > adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer), > adjustForAbsoluteZoom(renderer->scrollTop(), *renderer) > ); The better way to write this is auto originalPosition = FloatPoint { ... } > Source/WebCore/dom/Element.cpp:992 > + ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, originalPosition.x(), originalPosition.y()); auto > Source/WebCore/dom/Element.cpp:998 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); auto > Source/WebCore/dom/Element.cpp:1003 > + if (snapPointSelectionMethod == ScrollSnapPointSelectionMethod::Directional) > + setScrollPositionOptions.enableDirectionalScrollSnapping(originalPosition); > + if (useSmoothScrolling(scrollToOptions.behavior.valueOr(ScrollBehavior::Auto), this)) > + setScrollPositionOptions.setAnimated(); Is this code that could live in a helper on ScrollableArea? > Source/WebCore/dom/Element.cpp:1333 > + auto options = SetScrollPositionOptions::createProgrammatic(); > + if (useSmoothScrolling(ScrollBehavior::Auto, this)) > + options.setAnimated(); Ditto. Like ScrollableArea::optionsForProgrammaticScroll(). > Source/WebCore/page/DOMWindow.cpp:1708 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); auto. > Source/WebCore/page/FrameView.cpp:2331 > + ScrollOffset snappedOffset = ceiledIntPoint( > + scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(scrollOffsetFromPosition(scrollPosition), delta, originalPosition)); Weird wrapping > Source/WebCore/platform/ScrollTypes.h:257 > +enum class ScrollSnapPointSelectionMethod : bool { I prefer uint8_t rather than bool. > Source/WebCore/platform/ScrollTypes.h:270 > +struct SetScrollPositionOptions { Having "Set" in the name makes this read like a function at the call sites. Maybe ScrollPositionChangeOptions? > Source/WebCore/platform/cocoa/ScrollController.mm:918 > + Optional<LayoutUnit> originalPositionInLayoutUnits = WTF::nullopt; No need to initialize. > Source/WebCore/rendering/RenderLayer.cpp:2849 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(box->element(), options.behavior)) > + setScrollPositionOptions.setAnimated(); More sharable code. > Source/WebCore/rendering/RenderLayer.cpp:2884 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(element, options.behavior)) > + setScrollPositionOptions.setAnimated(); Ditto > Source/WebCore/rendering/RenderLayer.cpp:2935 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(element, options.behavior)) > + setScrollPositionOptions.setAnimated(); Ditto Created attachment 417448 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 417371 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417371&action=review Thanks for the review! > > > Source/WebCore/dom/Element.cpp:991 > > + FloatPoint originalPosition = FloatPoint( > > adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer), > > adjustForAbsoluteZoom(renderer->scrollTop(), *renderer) > > ); > > The better way to write this is auto originalPosition = FloatPoint { ... } I gave this a shot, but it seems that things inside curly initialisers and casts are subject to errors due to narrowing int into float. I've switched this to: FloatPoint originalPosition(...) > > Source/WebCore/dom/Element.cpp:992 > > + ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, originalPosition.x(), originalPosition.y()); > > auto Fixed. > > > Source/WebCore/dom/Element.cpp:998 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > auto Fixed. > > > Source/WebCore/dom/Element.cpp:1003 > > + if (snapPointSelectionMethod == ScrollSnapPointSelectionMethod::Directional) > > + setScrollPositionOptions.enableDirectionalScrollSnapping(originalPosition); > > + if (useSmoothScrolling(scrollToOptions.behavior.valueOr(ScrollBehavior::Auto), this)) > > + setScrollPositionOptions.setAnimated(); > > Is this code that could live in a helper on ScrollableArea? It's a little tricky to do a simple helper in ScrollableArea, but I reworked this a quite a bit and now I think there isn't any repeated code, apart from the call to useSmoothScrolling (which takes a different argument). > > > Source/WebCore/dom/Element.cpp:1333 > > + auto options = SetScrollPositionOptions::createProgrammatic(); > > + if (useSmoothScrolling(ScrollBehavior::Auto, this)) > > + options.setAnimated(); > > Ditto. Like ScrollableArea::optionsForProgrammaticScroll(). > > > Source/WebCore/page/DOMWindow.cpp:1708 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > auto. Fixed. > > > Source/WebCore/page/FrameView.cpp:2331 > > + ScrollOffset snappedOffset = ceiledIntPoint( > > + scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(scrollOffsetFromPosition(scrollPosition), delta, originalPosition)); > > Weird wrapping Fixed. > > > Source/WebCore/platform/ScrollTypes.h:257 > > +enum class ScrollSnapPointSelectionMethod : bool { > > I prefer uint8_t rather than bool. Fixed. > > > Source/WebCore/platform/ScrollTypes.h:270 > > +struct SetScrollPositionOptions { > > Having "Set" in the name makes this read like a function at the call sites. > Maybe ScrollPositionChangeOptions? Sure. I'm not at all attached to the original name. I've renamed it to ScrollPositionChangeOptions. > > > Source/WebCore/platform/cocoa/ScrollController.mm:918 > > + Optional<LayoutUnit> originalPositionInLayoutUnits = WTF::nullopt; > > No need to initialize. Fixed. > > > Source/WebCore/rendering/RenderLayer.cpp:2849 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > + if (shouldUseAnimatedScroll(box->element(), options.behavior)) > > + setScrollPositionOptions.setAnimated(); > > More sharable code. Fixed. Comment on attachment 417448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417448&action=review > Source/WebCore/platform/ScrollAnimator.cpp:375 > + Optional<float> originalX, originalY; > + FloatSize velocity; > + if (method == ScrollSnapPointSelectionMethod::Directional) { > + FloatSize scrollOrigin = toFloatSize(m_scrollableArea.scrollOrigin()); > + originalX = currentPosition().x(); > + originalY = currentPosition().y(); > + auto newPosition = ScrollableArea::scrollPositionFromOffset(offset, scrollOrigin); > + velocity = { newPosition.x() - currentPosition().x(), newPosition.y() - currentPosition().y() }; > + } > + > + FloatPoint newOffset = offset; > + newOffset.setX(m_scrollController.adjustScrollDestination(ScrollEventAxis::Horizontal, newOffset.x(), velocity.width(), originalX)); > + newOffset.setY(m_scrollController.adjustScrollDestination(ScrollEventAxis::Vertical, newOffset.y(), velocity.height(), originalY)); There's an unsetting mix of scrollOffset and scrollPosition here. originalX/originalY are positions (please name them thus), and are passed into m_scrollController.adjustScrollDestination(), but I can't tell if that actually treats them as positions or offsets. Please test in a horizontal RTL scroller. > Source/WebCore/platform/cocoa/ScrollController.mm:921 > + LayoutUnit offset = closestSnapOffset(snapState.snapOffsetsForAxis(axis), snapState.snapOffsetRangesForAxis(axis), LayoutUnit(destination / m_client.pageScaleFactor()), velocity, snapIndex, originalPositionInLayoutUnits); Does closestSnapOffset() treat originalPositionInLayoutUnits as a position or offset? > Source/WebCore/rendering/RenderLayer.cpp:2645 > + IntPoint snappedOffset = ceiledIntPoint(scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(clampedScrollOffset, options.snapPointSelectionMethod)); IntPoint -> ScrollOffset Created attachment 417520 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 417448 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417448&action=review > > > Source/WebCore/platform/ScrollAnimator.cpp:375 > > + Optional<float> originalX, originalY; > > + FloatSize velocity; > > + if (method == ScrollSnapPointSelectionMethod::Directional) { > > + FloatSize scrollOrigin = toFloatSize(m_scrollableArea.scrollOrigin()); > > + originalX = currentPosition().x(); > > + originalY = currentPosition().y(); > > + auto newPosition = ScrollableArea::scrollPositionFromOffset(offset, scrollOrigin); > > + velocity = { newPosition.x() - currentPosition().x(), newPosition.y() - currentPosition().y() }; > > + } > > + > > + FloatPoint newOffset = offset; > > + newOffset.setX(m_scrollController.adjustScrollDestination(ScrollEventAxis::Horizontal, newOffset.x(), velocity.width(), originalX)); > > + newOffset.setY(m_scrollController.adjustScrollDestination(ScrollEventAxis::Vertical, newOffset.y(), velocity.height(), originalY)); > > There's an unsetting mix of scrollOffset and scrollPosition here. > originalX/originalY are positions (please name them thus), and are passed > into m_scrollController.adjustScrollDestination(), but I can't tell if that > actually treats them as positions or offsets. Please test in a horizontal > RTL scroller. Agreed. After looking at this again, I think we should always be passing offsets here. I've done that and have modified all of the arguments and variable names down the call chain to make it clear that these are scroll offets. Additionally, I've written a test that verifies the correct scroll snap behavior for RTL scrollers when scrolling from script. That test is part of the patch now. > > Source/WebCore/rendering/RenderLayer.cpp:2645 > > + IntPoint snappedOffset = ceiledIntPoint(scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(clampedScrollOffset, options.snapPointSelectionMethod)); > > IntPoint -> ScrollOffset Fixed. Committed r271439: <https://trac.webkit.org/changeset/271439> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417520 [details]. Comment on attachment 417520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417520&action=review > Source/WebCore/page/ScrollBehavior.cpp:41 > + // FIXME: Should we use document()->scrollingElement()? > + // See https://bugs.webkit.org/show_bug.cgi?id=205059 > + if (associatedElement == associatedElement->document().scrollingElement()) > + associatedElement = associatedElement->document().documentElement(); This is dereferencing associatedElement *before* it is null checked below. |