Bug 225395 - AppHighlight scrolls should be smooth
Summary: AppHighlight scrolls should be smooth
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-05 09:45 PDT by Megan Gardner
Modified: 2021-05-05 23:43 PDT (History)
15 users (show)

See Also:


Attachments
Patch (10.41 KB, patch)
2021-05-05 11:42 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2021-05-05 12:37 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (14.02 KB, patch)
2021-05-05 16:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2021-05-05 09:45:45 PDT
AppHighlight scrolls should be smooth
Comment 1 Megan Gardner 2021-05-05 11:42:54 PDT
Created attachment 427786 [details]
Patch
Comment 2 Megan Gardner 2021-05-05 11:44:59 PDT
rdar://77517808
Comment 3 Tim Horton 2021-05-05 11:51:17 PDT
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 4 Devin Rousso 2021-05-05 11:56:51 PDT
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.
Comment 5 Megan Gardner 2021-05-05 12:37:57 PDT
Created attachment 427789 [details]
Patch
Comment 6 Tim Horton 2021-05-05 12:50:39 PDT
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 7 Simon Fraser (smfr) 2021-05-05 13:40:47 PDT
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.
Comment 8 Megan Gardner 2021-05-05 16:56:40 PDT
Created attachment 427823 [details]
Patch
Comment 9 EWS 2021-05-05 23:43:22 PDT
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].