autofocus should queue a task instead of firing synchronously

• Component: Forms
• Status: RESOLVED DUPLICATE

[Ojan Vafai]

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.

Created attachment 134978 [details]
Patch

[Ryosuke Niwa]

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

Created attachment 134991 [details]
Patch

all comments adressed

[WebKit Review Bot]

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]

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

(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

Created attachment 134999 [details]
Patch

[WebKit Review Bot]

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]

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

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

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

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

Created attachment 135189 [details]
slightly better test case

[Ojan Vafai]

Comment on attachment 135189 [details]
slightly better test case

ugh, nm. this test case is broken.

[Ojan Vafai]

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]

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.

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

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

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

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

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

[Ryosuke Niwa]

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