Bug 208566

Summary:

REGRESSION(r255957): Element with scroll-behavior:smooth isn't draggable after r255957

Product:

WebKit

Reporter:

cathiechen <cathiechen>

Component:

Scrolling

Assignee:

cathiechen <cathiechen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=42194

Bug Depends on:

219284

Bug Blocks:

204882

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
drag-smooth-scroll-element	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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>