Bug 208566 - REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable after r255957
Summary: REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable afte...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on: 219284
Blocks: 204882
  Show dependency treegraph
 
Reported: 2020-03-04 00:53 PST by cathiechen
Modified: 2020-11-25 07:12 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 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.
Comment 1 cathiechen 2020-03-04 02:17:00 PST
Created attachment 392397 [details]
Patch
Comment 2 cathiechen 2020-03-04 02:24:06 PST
Created attachment 392398 [details]
Patch
Comment 3 cathiechen 2020-03-05 23:24:05 PST
Created attachment 392685 [details]
Patch
Comment 4 cathiechen 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?
Comment 5 cathiechen 2020-03-05 23:34:14 PST
Created attachment 392686 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 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?
Comment 7 cathiechen 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.
Comment 8 cathiechen 2020-03-06 05:02:26 PST
Created attachment 392708 [details]
Patch
Comment 9 cathiechen 2020-03-06 05:28:17 PST
Created attachment 392710 [details]
Patch
Comment 10 Simon Fraser (smfr) 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
Comment 11 cathiechen 2020-03-06 10:00:52 PST
Created attachment 392738 [details]
drag-smooth-scroll-element

Add test case.
Comment 12 cathiechen 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 } ?
Comment 13 cathiechen 2020-03-06 11:22:04 PST
Created attachment 392747 [details]
Patch
Comment 14 cathiechen 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:)
Comment 15 cathiechen 2020-03-09 23:13:22 PDT
Created attachment 393121 [details]
Patch
Comment 16 Frédéric Wang (:fredw) 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.
Comment 17 cathiechen 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.
Comment 18 cathiechen 2020-03-11 09:39:34 PDT
Created attachment 393253 [details]
Patch
Comment 19 cathiechen 2020-03-12 08:34:23 PDT
Created attachment 393378 [details]
Patch
Comment 20 cathiechen 2020-03-12 21:44:07 PDT
Created attachment 393456 [details]
Patch
Comment 21 WebKit Commit Bot 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>
Comment 22 WebKit Commit Bot 2020-03-12 22:31:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2020-03-12 22:31:17 PDT
<rdar://problem/60404576>