WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
225395
AppHighlight scrolls should be smooth
https://bugs.webkit.org/show_bug.cgi?id=225395
Summary
AppHighlight scrolls should be smooth
Megan Gardner
Reported
2021-05-05 09:45:45 PDT
AppHighlight scrolls should be smooth
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2021-05-05 11:42:54 PDT
Created
attachment 427786
[details]
Patch
Megan Gardner
Comment 2
2021-05-05 11:44:59 PDT
rdar://77517808
Tim Horton
Comment 3
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?
Devin Rousso
Comment 4
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.
Megan Gardner
Comment 5
2021-05-05 12:37:57 PDT
Created
attachment 427789
[details]
Patch
Tim Horton
Comment 6
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.
Simon Fraser (smfr)
Comment 7
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.
Megan Gardner
Comment 8
2021-05-05 16:56:40 PDT
Created
attachment 427823
[details]
Patch
EWS
Comment 9
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug