Bug 203139 – Implement new autofocus behavior

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

autofocus processing model has been updated in https://github.com/whatwg/html/pull/4763 and https://github.com/whatwg/html/pull/5015

Implement that in WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/56397019>

[Ryosuke Niwa]

*** Bug 129743 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 82778 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 213982 has been marked as a duplicate of this bug. ***

[Kent Tamura]

*** Bug 202651 has been marked as a duplicate of this bug. ***

[Kent Tamura]

*** Bug 200913 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 438089 [details]
WIP

Here's some work-in-progress patch.

[Brent Fulgham]

Created attachment 439376 [details]
Patch

[Brent Fulgham]

Rebased patch for EWS.

[Tim Nguyen (:ntim)]

Relevant links:

https://html.spec.whatwg.org/multipage/interaction.html#the-autofocus-attribute

https://html.spec.whatwg.org/multipage/interaction.html#focusing-steps

[Tim Nguyen (:ntim)]

Created attachment 440195 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440204 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440211 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440213 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 440213 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Make WebKit match the new autofocus spec:

You should summarize how the old behavior differs from the new behavior here, and what code changes were necessary to implement the new behavior.

[Tim Nguyen (:ntim)]

Created attachment 440437 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440485 [details]
Patch

[Wenson Hsieh]

Comment on attachment 440485 [details]
Patch

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

Seems reasonable to me, but it would be good if Simon or Chris could take a look as well.

(Also, do we know why form-submission-crash-3.html is timing out on the Windows bot?)

> Source/WebCore/dom/Document.cpp:4481
> +    m_autofocusCandidates.appendOrMoveToLast(candidate);

Should we clear this out elsewhere as well? (e.g. `destroyRenderTree()` or `commonTeardown()`?)

> Source/WebCore/dom/Document.cpp:4491
> +    for (; !m_autofocusCandidates.isEmpty(); m_autofocusCandidates.removeFirst()) {
> +        Ref element = m_autofocusCandidates.first();

Nit - I think this might be a bit cleaner with a `while (!m_autofocusCandidates.isEmpty())` loop and `takeFirst()`.

[Tim Nguyen (:ntim)]

(In reply to Wenson Hsieh from comment #18)
> Comment on attachment 440485 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440485&action=review
> 
> Seems reasonable to me, but it would be good if Simon or Chris could take a
> look as well.
> 
> (Also, do we know why form-submission-crash-3.html is timing out on the
> Windows bot?)

Another race between the first rendering update and the onload event :)

> > Source/WebCore/dom/Document.cpp:4481
> > +    m_autofocusCandidates.appendOrMoveToLast(candidate);
> 
> Should we clear this out elsewhere as well? (e.g. `destroyRenderTree()` or
> `commonTeardown()`?)

I guess we could clear it in commonTeardown().

[Tim Nguyen (:ntim)]

Created attachment 440642 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440644 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440795 [details]
Patch

[Chris Dumez]

Comment on attachment 440795 [details]
Patch

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

> Source/WebCore/dom/Document.h:1809
> +    ListHashSet<Ref<Element>> m_autofocusCandidates;

Does this really need to hold a strong ref to the elements? Seems like it shouldn't and it would be less leak-prone to hold WeakPtrs.

> Source/WebCore/dom/Document.h:1810
> +    bool m_isAutofocusProcessed { false };

Please move this bool next to some other boolean data members for better packing.

[Tim Nguyen (:ntim)]

Created attachment 440812 [details]
Patch

[EWS]

Committed r283935 (242794@main): <https://commits.webkit.org/242794@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440812 [details].