Bug 203139 - Implement new autofocus behavior
Summary: Implement new autofocus behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
: 82778 129743 200913 202651 213982 (view as bug list)
Depends on: 231647 230851 231168 231219 231220 231323 231327 231357 231845
Blocks: 202843 231529 231530 231531 227537
  Show dependency treegraph
 
Reported: 2019-10-17 22:07 PDT by Ryosuke Niwa
Modified: 2021-10-19 15:31 PDT (History)
18 users (show)

See Also:


Attachments
WIP (35.87 KB, patch)
2021-09-13 16:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (44.57 KB, patch)
2021-09-27 11:54 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (35.02 KB, patch)
2021-10-05 03:47 PDT, Tim Nguyen (:ntim)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.98 KB, patch)
2021-10-05 05:54 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (39.09 KB, patch)
2021-10-05 07:42 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (40.14 KB, patch)
2021-10-05 07:48 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (43.09 KB, patch)
2021-10-06 14:16 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (44.87 KB, patch)
2021-10-07 00:55 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (46.02 KB, patch)
2021-10-08 10:41 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (45.99 KB, patch)
2021-10-08 10:50 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (45.77 KB, patch)
2021-10-11 08:08 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (45.94 KB, patch)
2021-10-11 11:55 PDT, Tim Nguyen (:ntim)
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 2019-10-17 22:07:56 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-10-17 22:09:20 PDT
<rdar://problem/56397019>
Comment 2 Ryosuke Niwa 2019-10-17 22:11:13 PDT
*** Bug 129743 has been marked as a duplicate of this bug. ***
Comment 3 Ryosuke Niwa 2019-10-17 22:11:26 PDT
*** Bug 82778 has been marked as a duplicate of this bug. ***
Comment 4 Ryosuke Niwa 2020-09-26 15:25:25 PDT
*** Bug 213982 has been marked as a duplicate of this bug. ***
Comment 5 Kent Tamura 2020-12-09 00:34:40 PST
*** Bug 202651 has been marked as a duplicate of this bug. ***
Comment 6 Kent Tamura 2020-12-09 00:35:06 PST
*** Bug 200913 has been marked as a duplicate of this bug. ***
Comment 7 Ryosuke Niwa 2021-09-13 16:38:33 PDT
Created attachment 438089 [details]
WIP

Here's some work-in-progress patch.
Comment 8 Brent Fulgham 2021-09-27 11:54:11 PDT
Created attachment 439376 [details]
Patch
Comment 9 Brent Fulgham 2021-09-27 11:54:47 PDT
Rebased patch for EWS.
Comment 11 Tim Nguyen (:ntim) 2021-10-05 03:47:59 PDT
Created attachment 440195 [details]
Patch
Comment 12 Tim Nguyen (:ntim) 2021-10-05 05:54:15 PDT
Created attachment 440204 [details]
Patch
Comment 13 Tim Nguyen (:ntim) 2021-10-05 07:42:30 PDT
Created attachment 440211 [details]
Patch
Comment 14 Tim Nguyen (:ntim) 2021-10-05 07:48:23 PDT
Created attachment 440213 [details]
Patch
Comment 15 Simon Fraser (smfr) 2021-10-05 09:01:50 PDT
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.
Comment 16 Tim Nguyen (:ntim) 2021-10-06 14:16:44 PDT
Created attachment 440437 [details]
Patch
Comment 17 Tim Nguyen (:ntim) 2021-10-07 00:55:06 PDT
Created attachment 440485 [details]
Patch
Comment 18 Wenson Hsieh 2021-10-07 19:58:22 PDT
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()`.
Comment 19 Tim Nguyen (:ntim) 2021-10-08 00:29:02 PDT
(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().
Comment 20 Tim Nguyen (:ntim) 2021-10-08 10:41:12 PDT
Created attachment 440642 [details]
Patch
Comment 21 Tim Nguyen (:ntim) 2021-10-08 10:50:30 PDT
Created attachment 440644 [details]
Patch
Comment 22 Tim Nguyen (:ntim) 2021-10-11 08:08:21 PDT
Created attachment 440795 [details]
Patch
Comment 23 Chris Dumez 2021-10-11 08:36:41 PDT
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.
Comment 24 Tim Nguyen (:ntim) 2021-10-11 11:55:34 PDT
Created attachment 440812 [details]
Patch
Comment 25 EWS 2021-10-11 13:59:51 PDT
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].