Bug 181575 – Make scrolling to the focused element async

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

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().

[Radar WebKit Bug Importer]

<rdar://problem/36459767>

[Ryosuke Niwa]

Created attachment 332127 [details]
WIP

[EWS Watchlist]

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]

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 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]

Created attachment 332325 [details]
Changes the behavior

[Simon Fraser (smfr)]

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]

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 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]

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]

Created attachment 332338 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 332338 [details]
Patch for landing

Will look into the iOS test failure.

[Ryosuke Niwa]

Created attachment 332345 [details]
Patch for landing

[Ryosuke Niwa]

Fixed the test for iOS. Waiting for EWS now.

[Ryosuke Niwa]

Committed r227664: <https://trac.webkit.org/changeset/227664>