Bug 181575 - Make scrolling to the focused element async
Summary: Make scrolling to the focused element async
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-11 20:27 PST by Ryosuke Niwa
Modified: 2018-01-26 04:20 PST (History)
21 users (show)

See Also:


Attachments
WIP (19.29 KB, patch)
2018-01-23 23:43 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.15 MB, application/zip)
2018-01-24 01:18 PST, EWS Watchlist
no flags Details
Changes the behavior (28.93 KB, patch)
2018-01-25 15:33 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.14 MB, application/zip)
2018-01-25 17:08 PST, EWS Watchlist
no flags Details
Patch for landing (29.04 KB, patch)
2018-01-25 17:24 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (29.03 KB, patch)
2018-01-25 19:40 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-01-11 20:27:00 PST
Right now, Element::focus() synchronously scrolls to the focused element.
Make the scrolling async so that we don't have to synchronously update the layout in Element::focus().
Comment 1 Radar WebKit Bug Importer 2018-01-11 20:28:10 PST
<rdar://problem/36459767>
Comment 2 Ryosuke Niwa 2018-01-23 23:43:05 PST
Created attachment 332127 [details]
WIP
Comment 3 EWS Watchlist 2018-01-24 01:18:56 PST
Comment on attachment 332127 [details]
WIP

Attachment 332127 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6191954

New failing tests:
fast/text/combining-character-sequence-fallback-crash.html
Comment 4 EWS Watchlist 2018-01-24 01:18:58 PST
Created attachment 332136 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 5 Ryosuke Niwa 2018-01-24 12:08:11 PST
Comment on attachment 332136 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

I don't think this test failure is related to my patch.
Comment 6 Ryosuke Niwa 2018-01-25 15:33:11 PST
Created attachment 332325 [details]
Changes the behavior
Comment 7 Simon Fraser (smfr) 2018-01-25 16:09:04 PST
Comment on attachment 332325 [details]
Changes the behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=332325&action=review

> Source/WebCore/page/FrameView.cpp:270
> +    m_shouldScrollToFocusedElement = false;
> +    m_delayedScrollToFocusedElementTimer.stop();

If it's always true that m_shouldScrollToFocusedElement == m_delayedScrollToFocusedElementTimer.stop.isActive(), then you can remove the bool.

> Source/WebCore/page/FrameView.cpp:2274
> +    scrollToFocusedElementTimerFired();

I'd prefer to see the guts of scrollToFocusedElementTimerFired() in a new function that you call here, rather than confusingly calling scrollToFocusedElementTimerFired().
Comment 8 Ryosuke Niwa 2018-01-25 17:06:07 PST
Thanks for the review.

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 332325 [details]
> Changes the behavior
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332325&action=review
> 
> > Source/WebCore/page/FrameView.cpp:270
> > +    m_shouldScrollToFocusedElement = false;
> > +    m_delayedScrollToFocusedElementTimer.stop();
> 
> If it's always true that m_shouldScrollToFocusedElement ==
> m_delayedScrollToFocusedElementTimer.stop.isActive(), then you can remove
> the bool.

It's possible that updateLayoutIgnorePendingStylesheets would execute scripts and call scheduleScrollToFocusedElement, in which case m_shouldScrollToFocusedElement is true but m_delayedScrollToFocusedElementTimer is not active (since it has already fired).

> 
> > Source/WebCore/page/FrameView.cpp:2274
> > +    scrollToFocusedElementTimerFired();
> 
> I'd prefer to see the guts of scrollToFocusedElementTimerFired() in a new
> function that you call here, rather than confusingly calling
> scrollToFocusedElementTimerFired().

Sure.
Comment 9 EWS Watchlist 2018-01-25 17:08:04 PST
Comment on attachment 332325 [details]
Changes the behavior

Attachment 332325 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6214469

New failing tests:
fast/scrolling/scroll-to-focused-element-asynchronously.html
Comment 10 EWS Watchlist 2018-01-25 17:08:06 PST
Created attachment 332337 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 11 Ryosuke Niwa 2018-01-25 17:24:37 PST
Created attachment 332338 [details]
Patch for landing
Comment 12 Ryosuke Niwa 2018-01-25 17:24:52 PST
Comment on attachment 332338 [details]
Patch for landing

Will look into the iOS test failure.
Comment 13 Ryosuke Niwa 2018-01-25 19:40:37 PST
Created attachment 332345 [details]
Patch for landing
Comment 14 Ryosuke Niwa 2018-01-25 19:42:00 PST
Fixed the test for iOS. Waiting for EWS now.
Comment 15 Ryosuke Niwa 2018-01-25 21:03:39 PST
Committed r227664: <https://trac.webkit.org/changeset/227664>