Bug 181575

Summary: Make scrolling to the focused element async
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, bfulgham, cdumez, cfleizach, cmarcelo, commit-queue, dbates, dmazzoni, esprehn+autocc, ews-watchlist, fred.wang, gyuyoung.kim, japhet, jcraig, jdiggs, kangil.han, samuel_white, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Changes the behavior
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch for landing
none
Patch for landing none

Ryosuke Niwa
Reported 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().
Attachments
WIP (19.29 KB, patch)
2018-01-23 23:43 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch for landing (29.03 KB, patch)
2018-01-25 19:40 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-01-11 20:28:10 PST
<rdar://problem/36459767>
Ryosuke Niwa
Comment 2 2018-01-23 23:43:05 PST
Created attachment 332127 [details] WIP
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
Ryosuke Niwa
Comment 5 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.
Ryosuke Niwa
Comment 6 2018-01-25 15:33:11 PST
Created attachment 332325 [details] Changes the behavior
Simon Fraser (smfr)
Comment 7 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().
Ryosuke Niwa
Comment 8 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.
EWS Watchlist
Comment 9 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
EWS Watchlist
Comment 10 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
Ryosuke Niwa
Comment 11 2018-01-25 17:24:37 PST
Created attachment 332338 [details] Patch for landing
Ryosuke Niwa
Comment 12 2018-01-25 17:24:52 PST
Comment on attachment 332338 [details] Patch for landing Will look into the iOS test failure.
Ryosuke Niwa
Comment 13 2018-01-25 19:40:37 PST
Created attachment 332345 [details] Patch for landing
Ryosuke Niwa
Comment 14 2018-01-25 19:42:00 PST
Fixed the test for iOS. Waiting for EWS now.
Ryosuke Niwa
Comment 15 2018-01-25 21:03:39 PST
Committed r227664: <https://trac.webkit.org/changeset/227664>
Note You need to log in before you can comment on or make changes to this bug.