Bug 225395 – AppHighlight scrolls should be smooth

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

AppHighlight scrolls should be smooth

[Megan Gardner]

Created attachment 427786 [details]
Patch

[Megan Gardner]

rdar://77517808

[Tim Horton]

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?

[Devin Rousso]

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.

[Megan Gardner]

Created attachment 427789 [details]
Patch

[Tim Horton]

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.

[Simon Fraser (smfr)]

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.

[Megan Gardner]

Created attachment 427823 [details]
Patch

[EWS]

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].