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().
<rdar://problem/36459767>
Created attachment 332127 [details] WIP
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
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 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.
Created attachment 332325 [details] Changes the behavior
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().
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 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
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
Created attachment 332338 [details] Patch for landing
Comment on attachment 332338 [details] Patch for landing Will look into the iOS test failure.
Created attachment 332345 [details] Patch for landing
Fixed the test for iOS. Waiting for EWS now.
Committed r227664: <https://trac.webkit.org/changeset/227664>