WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
208566
REGRESSION(
r255957
): Element with scroll-behavior:smooth isn't draggable after
r255957
https://bugs.webkit.org/show_bug.cgi?id=208566
Summary
REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable afte...
cathiechen
Reported
2020-03-04 00:53:55 PST
The autoscroll uses m_autoscrollTimer to perform drag scroll animation, then uses RenderLayer::scrollRectToVisible to perform scroll. In this scenario RenderLayer::scrollRectToVisible shouldn't consider scroll-behavior and scroll immediately.
Attachments
Patch
(6.14 KB, patch)
2020-03-04 02:17 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(6.12 KB, patch)
2020-03-04 02:24 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(6.26 KB, patch)
2020-03-05 23:24 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(6.26 KB, patch)
2020-03-05 23:34 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(6.27 KB, patch)
2020-03-06 05:02 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(6.39 KB, patch)
2020-03-06 05:28 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
drag-smooth-scroll-element
(885 bytes, text/html)
2020-03-06 10:00 PST
,
cathiechen
no flags
Details
Patch
(12.59 KB, patch)
2020-03-06 11:22 PST
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(12.59 KB, patch)
2020-03-09 23:13 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(12.95 KB, patch)
2020-03-11 09:39 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(11.13 KB, patch)
2020-03-12 08:34 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
Patch
(11.16 KB, patch)
2020-03-12 21:44 PDT
,
cathiechen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
cathiechen
Comment 1
2020-03-04 02:17:00 PST
Created
attachment 392397
[details]
Patch
cathiechen
Comment 2
2020-03-04 02:24:06 PST
Created
attachment 392398
[details]
Patch
cathiechen
Comment 3
2020-03-05 23:24:05 PST
Created
attachment 392685
[details]
Patch
cathiechen
Comment 4
2020-03-05 23:29:38 PST
About the test for this patch, we need to create a drag action. I tried to use eventSender, but it doesn't work. Maybe we can add a manual test?
cathiechen
Comment 5
2020-03-05 23:34:14 PST
Created
attachment 392686
[details]
Patch
Frédéric Wang (:fredw)
Comment 6
2020-03-06 03:35:14 PST
Comment on
attachment 392686
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=392686&action=review
LGTM, I wonder if we already have tests for this feature and whether we can adapt them for scroll behavior?
> Source/WebCore/rendering/RenderLayer.cpp:2797 > +void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions& options, bool isAutoscrollInProgress)
I guess it should be AutoScrollInProgress?
cathiechen
Comment 7
2020-03-06 04:57:44 PST
(In reply to Frédéric Wang (:fredw) from
comment #6
)
> Comment on
attachment 392686
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=392686&action=review
> > LGTM, I wonder if we already have tests for this feature and whether we can > adapt them for scroll behavior? >
Good idea! I'll have a try!
> > Source/WebCore/rendering/RenderLayer.cpp:2797 > > +void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insideFixed, const ScrollRectToVisibleOptions& options, bool isAutoscrollInProgress) > > I guess it should be AutoScrollInProgress?
Done, changed it to autoscrollInProgress.
cathiechen
Comment 8
2020-03-06 05:02:26 PST
Created
attachment 392708
[details]
Patch
cathiechen
Comment 9
2020-03-06 05:28:17 PST
Created
attachment 392710
[details]
Patch
Simon Fraser (smfr)
Comment 10
2020-03-06 09:22:18 PST
Comment on
attachment 392710
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=392710&action=review
> Source/WebCore/rendering/RenderLayer.cpp:2802 > + if (autoscrollInProgress) > + ASSERT(false);
hmm?
> Source/WebCore/rendering/RenderLayer.cpp:3040 > + scrollRectToVisible(LayoutRect(currentDocumentPosition, LayoutSize(1, 1)), false, { SelectionRevealMode::Reveal, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded, ShouldAllowCrossOriginScrolling::Yes }, true);
Boolean trap
cathiechen
Comment 11
2020-03-06 10:00:52 PST
Created
attachment 392738
[details]
drag-smooth-scroll-element Add test case.
cathiechen
Comment 12
2020-03-06 11:11:01 PST
Comment on
attachment 392710
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=392710&action=review
Hi Simon, Thanks for the review:)
>> Source/WebCore/rendering/RenderLayer.cpp:2802 >> + ASSERT(false); > > hmm?
Ah, sorry, this is used to find out the autoscroll tests. It'll remove in the new patch.
>> Source/WebCore/rendering/RenderLayer.cpp:3040 >> + scrollRectToVisible(LayoutRect(currentDocumentPosition, LayoutSize(1, 1)), false, { SelectionRevealMode::Reveal, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignToEdgeIfNeeded, ShouldAllowCrossOriginScrolling::Yes }, true); > > Boolean trap
Thanks, how about enum AutoscrollStatus { NotInProgress, InProgress } ?
cathiechen
Comment 13
2020-03-06 11:22:04 PST
Created
attachment 392747
[details]
Patch
cathiechen
Comment 14
2020-03-09 23:01:27 PDT
Hi Fred and Simon, The new patch add a test using eventSender to simulate the drag action. Please take a look, thanks:)
cathiechen
Comment 15
2020-03-09 23:13:22 PDT
Created
attachment 393121
[details]
Patch
Frédéric Wang (:fredw)
Comment 16
2020-03-10 03:58:40 PDT
Comment on
attachment 393121
[details]
Patch This LGTM, but I think it would be better to have a test for WK2. Can't you use UIHelper (e.g. dragFromPointToPoint?). See how it is done for other tests in fast/scrolling.
cathiechen
Comment 17
2020-03-11 08:34:37 PDT
(In reply to Frédéric Wang (:fredw) from
comment #16
)
> Comment on
attachment 393121
[details]
> Patch > > This LGTM, but I think it would be better to have a test for WK2. Can't you > use UIHelper (e.g. dragFromPointToPoint?). See how it is done for other > tests in fast/scrolling.
Hi Fred, Thanks for the review! It seems UIScriptControllerMac::dragFromPointToPoint for WebKitTestRunner is not implemented yet. I found UIScriptControllerMac::activateAtPoint which is related to mouse event and has implemented, is also using eventSender. I also looked into some examples of autoscroll tests, they are also using eventSender. So I guess eventSender is the common way to trigger an autoscroll in test. Bug:42194 is tracing the incomplete implementation for eventSender. So the test would cover WK2 if Bug:42194 fixed. Sorry, it seems my comments in changeLogs is confusing. I'll reword them.
cathiechen
Comment 18
2020-03-11 09:39:34 PDT
Created
attachment 393253
[details]
Patch
cathiechen
Comment 19
2020-03-12 08:34:23 PDT
Created
attachment 393378
[details]
Patch
cathiechen
Comment 20
2020-03-12 21:44:07 PDT
Created
attachment 393456
[details]
Patch
WebKit Commit Bot
Comment 21
2020-03-12 22:30:58 PDT
Comment on
attachment 393456
[details]
Patch Clearing flags on attachment: 393456 Committed
r258382
: <
https://trac.webkit.org/changeset/258382
>
WebKit Commit Bot
Comment 22
2020-03-12 22:31:00 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 23
2020-03-12 22:31:17 PDT
<
rdar://problem/60404576
>
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