AppHighlight scrolls should be smooth
Created attachment 427786 [details] Patch
rdar://77517808
Comment on attachment 427786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427786&action=review > Source/WebCore/editing/FrameSelection.cpp:438 > + if (options & SmoothScroll) > + m_scrollBehavior = ScrollBehavior::Smooth; What resets this? Maybe you want `m_scrollBehavior = options & SmoothScroll ? ScrollBehavior::Smooth : ScrollBehavior::Instant;`? Otherwise you're going to get stuck with smooth scrolling if you do it once. It's also kind of too bad we have to store it. The plumbing down to updateAndRevealSelection was too much?
Comment on attachment 427786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427786&action=review > Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:266 > OptionSet<TemporarySelectionOption> temporarySelectionOptions; > temporarySelectionOptions.add(TemporarySelectionOption::RevealSelection); > + temporarySelectionOptions.add(TemporarySelectionOption::SmoothScroll); NIT: you could inline this as `{ TemporarySelectionOption::RevealSelection, TemporarySelectionOption::SmoothScroll }` > Source/WebCore/editing/FrameSelection.cpp:438 > + if (options & SmoothScroll) > + m_scrollBehavior = ScrollBehavior::Smooth; Should this have an `else` to reset it back to `ScrollBehavior::Instant`? > Source/WebCore/page/ScrollBehavior.cpp:46 > - if (!associatedElement->renderer() || !associatedElement->document().settings().CSSOMViewSmoothScrollingEnabled()) > + if (!associatedElement->renderer()) Would this also affect things like `window.scrollTo`? If so, I don't think we want that.
Created attachment 427789 [details] Patch
Comment on attachment 427789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427789&action=review > Source/WebCore/page/ScrollBehavior.cpp:53 > + if (!associatedElement->document().settings().CSSOMViewSmoothScrollingEnabled()) > + return false; This seems *right*, but it is confusing. You're using the passed-in behavior unconditionally, unless it's auto, in which case you use the element's style's behavior only if the bit is on, right? And then making sure that script doesn't pass smooth if the setting is off at that callsite? I think it's OK. Maybe want smfr to peek.
Comment on attachment 427789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427789&action=review > Source/WebCore/ChangeLog:8 > + Leveral smooth scrolling experimental feature to allow AppHighlight scrolls Leveral? Leverage? > Source/WebCore/editing/FrameSelection.cpp:437 > + m_scrollBehavior = options & SmoothScroll ? ScrollBehavior::Smooth : ScrollBehavior::Instant; options.contains()? > Source/WebCore/editing/FrameSelection.h:340 > + ScrollBehavior m_scrollBehavior { ScrollBehavior::Instant }; It's a shame we need to store this. Can we just pass it along in parameters everywhere? Storing it here leaves us open to bugs where it gets left enabled by mistake. > Source/WebCore/page/DOMWindow.cpp:1730 > + auto scrollOptions = document()->settings().CSSOMViewSmoothScrollingEnabled() ? scrollToOptions.behavior.valueOr(ScrollBehavior::Auto) : ScrollBehavior::Auto; > + auto animated = useSmoothScrolling(scrollOptions, document()->documentElement()) ? AnimatedScroll::Yes : AnimatedScroll::No; This could use a changelog comment. useSmoothScrolling() consults CSSOMViewSmoothScrollingEnabled internally so it's very confusing to see that check also on the line above. >> Source/WebCore/page/ScrollBehavior.cpp:53 >> + return false; > > This seems *right*, but it is confusing. You're using the passed-in behavior unconditionally, unless it's auto, in which case you use the element's style's behavior only if the bit is on, right? And then making sure that script doesn't pass smooth if the setting is off at that callsite? I think it's OK. Maybe want smfr to peek. I'm also confused since the call site also tests CSSOMViewSmoothScrollingEnabled. I think this function should be the only one to check CSSOMViewSmoothScrollingEnabled(), and should just get passed to it whatever is necessary to compute the result.
Created attachment 427823 [details] Patch
Committed r277069 (237372@main): <https://commits.webkit.org/237372@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427823 [details].