82778 – autofocus should queue a task instead of firing synchronously

RESOLVED DUPLICATE of bug 203139 82778

autofocus should queue a task instead of firing synchronously

https://bugs.webkit.org/show_bug.cgi?id=82778

Summary autofocus should queue a task instead of firing synchronously

Ojan Vafai

Reported 2012-03-30 14:07:24 PDT

Created attachment 134879 [details] test case As per the last step of http://www.whatwg.org/specs/web-apps/current-work/#autofocusing-a-form-control autofocus should queue a task. In WebKit it fires synchronously. Firefox correctly fires it async. We should do the same. Sync events that fire on DOM mutations are bad! I didn't test IE/Opera, but it seems pretty clear we should change to match the spec here.

Attachments
test case (488 bytes, text/html) 2012-03-30 14:07 PDT, Ojan Vafai	no flags	Details
Patch (6.06 KB, patch) 2012-03-31 13:30 PDT, jochen	no flags	Details Formatted Diff Diff
Patch (6.07 KB, patch) 2012-04-01 02:08 PDT, jochen	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (7.48 MB, application/zip) 2012-04-01 02:50 PDT, WebKit Review Bot	no flags	Details
Patch (20.22 KB, patch) 2012-04-01 08:26 PDT, jochen	abarth: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (7.27 MB, application/zip) 2012-04-01 09:17 PDT, WebKit Review Bot	no flags	Details
slightly better test case (1.00 KB, text/html) 2012-04-02 14:32 PDT, Ojan Vafai	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

jochen

Comment 1 2012-03-31 13:30:18 PDT

Created attachment 134978 [details] Patch

Ryosuke Niwa

Comment 2 2012-03-31 13:50:00 PDT

Comment on attachment 134978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134978&action=review > Source/WebCore/html/HTMLFormControlElement.cpp:209 > ref(); No need to ref. > Source/WebCore/html/HTMLFormControlElement.cpp:515 > + deref(); No need to deref. > LayoutTests/fast/events/autofocus-async-expected.txt:2 > +Checks that the focus event triggered by an auto-focused input element is send asynchronously. The test passes, if "timeout" is printed before "focus". > +timeout Instead of saying in which order things should happen, we should verify that in the test. > LayoutTests/fast/events/autofocus-async.html:17 > + if (window.layoutTestController) > + layoutTestController.notifyDone(); This test is going to timeout if autofocus stopped working. We should schedule some other event that logs something like "FAIL - never focused" with non-zero timeout.

Kent Tamura

Comment 3 2012-03-31 19:45:28 PDT

Comment on attachment 134978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134978&action=review > Source/WebCore/html/HTMLFormControlElement.h:181 > + Timer<HTMLFormControlElement> m_focusEventTimer; Timer is nota a small object. We had better use OwnPtr<Timer<>> and create it on demand.

jochen

Comment 4 2012-04-01 02:08:12 PDT

Created attachment 134991 [details] Patch

jochen

Comment 5 2012-04-01 02:08:38 PDT

all comments adressed

WebKit Review Bot

Comment 6 2012-04-01 02:49:58 PDT

Comment on attachment 134991 [details] Patch Attachment 134991 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12307152 New failing tests: fast/css/text-overflow-input-focus-placeholder.html fast/forms/autofocus-keygen.html fast/events/autofocus-async.html fast/forms/autofocus-input-css-style-change.html fast/forms/autofocus-opera-002.html fast/css/text-overflow-input-focus-value.html fast/forms/autofocus-opera-006.html fast/forms/number/number-placeholder-with-unacceptable-value.html fast/forms/autofocus-opera-003.html fast/forms/autofocus-opera-001.html fast/forms/textfield-no-linebreak.html fast/forms/autofocus-opera-007.html fast/forms/autofocus-readonly-attribute.html

WebKit Review Bot

Comment 7 2012-04-01 02:50:06 PDT

Created attachment 134992 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

jochen

Comment 8 2012-04-01 04:18:14 PDT

(In reply to comment #7) > Created an attachment (id=134992) [details] > Archive of layout-test-results from ec2-cr-linux-04 > > The attached test failures were seen while running run-webkit-tests on the chromium-ews. > Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick These tests are failing because the focus event is now fired after the load event. I guess that's the desired behavior, so I'll update the layout tests accordingly

jochen

Comment 9 2012-04-01 08:26:06 PDT

Created attachment 134999 [details] Patch

WebKit Review Bot

Comment 10 2012-04-01 09:17:40 PDT

Comment on attachment 134999 [details] Patch Attachment 134999 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12311131 New failing tests: fast/events/autofocus-async.html fast/forms/number/number-placeholder-with-unacceptable-value.html fast/forms/textfield-no-linebreak.html

WebKit Review Bot

Comment 11 2012-04-01 09:17:46 PDT

Created attachment 135003 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Ryosuke Niwa

Comment 12 2012-04-01 18:01:16 PDT

Comment on attachment 134999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134999&action=review > LayoutTests/fast/events/autofocus-async.html:36 > + }, 1000); One second is a really long time to wait for this. I'd say 50 should be enough; or even 1 might work. > LayoutTests/fast/forms/autofocus-opera-001.html:29 > -<body onload="test()"> > +<body onload="setTimeout(test, 0)"> Do we really want autofocus to happen after the load event? Does this behavior agree with Firefox and IE?

jochen

Comment 13 2012-04-02 00:14:38 PDT

(In reply to comment #12) > Do we really want autofocus to happen after the load event? Does this behavior agree with Firefox and IE? I tested this on Firefox, and Firefox fires the focus event before the load event. However, it also fires the timeout before the load event. WebKit fires both after the load event.

Adam Barth

Comment 14 2012-04-02 00:36:07 PDT

Comment on attachment 134999 [details] Patch I agree with rniwa that we shouldn't change the order of the autofocus and the load event. That's likely to cause compat trouble.

Ryosuke Niwa

Comment 15 2012-04-02 01:16:26 PDT

(In reply to comment #13) > (In reply to comment #12) > > Do we really want autofocus to happen after the load event? Does this behavior agree with Firefox and IE? > > I tested this on Firefox, and Firefox fires the focus event before the load event. However, it also fires the timeout before the load event. WebKit fires both after the load event. We should definitely figure out why Firefox does this. I'm pretty certain this behavior is spec'ed in HTML5/DOM 4 somewhere.

jochen

Comment 16 2012-04-02 06:20:24 PDT

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #12) > > > Do we really want autofocus to happen after the load event? Does this behavior agree with Firefox and IE? > > > > I tested this on Firefox, and Firefox fires the focus event before the load event. However, it also fires the timeout before the load event. WebKit fires both after the load event. > > We should definitely figure out why Firefox does this. I'm pretty certain this behavior is spec'ed in HTML5/DOM 4 somewhere. So looking at http://www.whatwg.org/specs/web-apps/current-work/#the-end we seem to deviate when dispatching the DOMContentLoaded event? Step 4 says that a task should be queued to dispatch the DOMContentLoaded event, however, we do this synchronously. Also, step 7 is synchronously in WebKit Would you agree that these two steps should be asynchronously? I guess that would make the timeout and autofocus events being dispatched before the load event

Ojan Vafai

Comment 17 2012-04-02 14:32:41 PDT

Created attachment 135189 [details] slightly better test case

Ojan Vafai

Comment 18 2012-04-02 14:33:52 PDT

Comment on attachment 135189 [details] slightly better test case ugh, nm. this test case is broken.

Ojan Vafai

Comment 19 2012-04-02 14:34:34 PDT

FWIW, Gecko sometimes fires the focus after the load event in this test case and IE10 seems to always fire it after the load event.

Ojan Vafai

Comment 20 2012-04-02 14:37:44 PDT

Maybe we can get 90% of the way there and get the benefits of avoiding sync events during DOM mutation by firing this event when the microtask completes. Raf, Adam, WDYT? Would this be hard to do? Is it gross? We can put a FIXME in to make it properly queue a task once bug 82931 is fixed.

jochen

Comment 21 2012-04-02 14:38:49 PDT

(In reply to comment #20) > Maybe we can get 90% of the way there and get the benefits of avoiding sync events during DOM mutation by firing this event when the microtask completes. Raf, Adam, WDYT? Would this be hard to do? Is it gross? > > We can put a FIXME in to make it properly queue a task once bug 82931 is fixed. I'd propose to make the autofocus just delay the load event until 82931 is fixed

Adam Barth

Comment 22 2012-04-02 14:40:02 PDT

> Raf, Adam, WDYT? I would queue the event and then delay the load event if there's a focus event in the queue. Ideally, we'd have all the events that the spec says go through the queue actually go through the queue, but that's probably more work than necessary to fix this bug. > I'd propose to make the autofocus just delay the load event until 82931 is fixed Makes sense to me.

Ryosuke Niwa

Comment 23 2012-04-02 15:04:53 PDT

(In reply to comment #20) > Maybe we can get 90% of the way there and get the benefits of avoiding sync events during DOM mutation by firing this event when the microtask completes. Raf, Adam, WDYT? Would this be hard to do? Is it gross? I don't think inventing our own timing is a good idea. If anything, we should figure out what exactly IE and FF do and match either behavior. (In reply to comment #21) > I'd propose to make the autofocus just delay the load event until 82931 is fixed I don't think we should try to "fix" this bug until the bug 82931 is fixed.

Ojan Vafai

Comment 24 2012-04-03 14:13:49 PDT

> (In reply to comment #21) > I don't think we should try to "fix" this bug until the bug 82931 is fixed. I disagree. Bug 82931 is considerably more complicated than this one. In the meantime, fixing this bug means we are closer to removing sync events during DOM mutation and thus greatly improving a lot of the C++ code. It also means we are more compatible with other browser vendors and the spec.

Ryosuke Niwa

Comment 25 2019-10-17 22:08:34 PDT

autofocus model has been overhauled. We're going to track this in https://bugs.webkit.org/show_bug.cgi?id=203139 instead.

Ryosuke Niwa

Comment 26 2019-10-17 22:11:26 PDT

*** This bug has been marked as a duplicate of bug 203139 ***

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution DUPLICATE

of bug 203139

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Forms

Assignee

jochen

Reported

2012-03-30 14:07 PDT

Modified

2019-10-17 22:11 PDT History

CC List

13 users Show

URL

Keywords

Depends on

Blocks

202843 202651

Dependencies

tree graph