569399 - autofocus sometimes does not work because the element has no frame yet

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Mounir Lamouri (:mounir)]

Open a page with autofocus on a form control [1]. The control should have the autofocus when the page is loaded, as expected but if you reload the page, the control will loose the focus which is not the expected behavior.

[1] for example: http://diveintohtml5.org/examples/input-autofocus.html

Comment 1

[Mounir Lamouri (:mounir)]

I did some checks and it look like the focus is stopped in |CheckIfFocusable|: it checks if the content has a frame which is not the case so it is considered as not focusable. Enn, there is a way to prevent that ?

Comment 2

[Neil Deakin]

Why is no frame present? The page hasn't been laid out yet?

Comment 3

[Rob Crowther]

I find the current nightly does trigger the autofocus if you reload the page, but doesn't autofocus if you're transitioning forward and backwards through history.  I'm assuming this bug can be closed and I should open a new one about that, but since there's been no activity on this one for 5 months I thought I'd check first.

Comment 4

[Mounir Lamouri (:mounir)]

Actually, autofocus has a lot of troubles and depending of the test case it might work or not. For example, the url in comment 0 doesn't seem to work with the current nightly.

However, I don't think autofocus should apply when navigating through the history given that the page isn't loaded and is even restored to the previous state.

Comment 5

[Rob Crowther]

In the particular instance where I came across this I'm using pageshow and pagehide to reset the form when the user goes backwards and forwards.  I'll admit my use case is probably non-standard, but it just seems a little awkward that I have to manually focus the element.  It does seem to get focussed automatically in Chrome, so this is maybe something that should be considered in the spec?

Also, it might be good if the form reset method/event triggered the autofocus routine?

Comment 6

[Johnny Stenback (:jst)]

We need to block on this, but if there's no easy fix then the resolution here for Firefox 4 is to disable autofocus and fix the issue later on.

Comment 8

[Mounir Lamouri (:mounir)]

Flushing the layout when we run the autofocus event doesn't change anything given that we already flush it when focus is called (see nsFocusManager::CheckIfFocusable).

So, if you do:
<input><script>document.getElementsByTagName('input')[0].focus();</script>
input will be focused. But if you do:
<input autofocus>
input will not be focused (in a situation were autofocus doesn't work).

I'm wondering if we are not sending the autofocus event too early and the frame creation request might not have been done yet so flushing would be worthless. Where/when is this managed?

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Testcase

1. Load the test case.

Expected result: the second input element is focused.
Actual result: the first input element is focused.

If you remove the <link>, it's working as expected.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Should we move nsAutoFocusEvent to layout, so that it would be dispatched when
the frame for the form control element is created?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> Should we move nsAutoFocusEvent to layout, so that it would be dispatched when
> the frame for the form control element is created?

If we are sure that in the attached testcase, .focus() isn't done before the autofocus, it would probably fix our issue. However, I still don't understand why the autofocus doesn't flush but .focus does...

Comment 12

[Mounir Lamouri (:mounir)]

So, it looks like flushing works correctly when there is no external stylesheet (or network load?): the frames are created when ::Focus is called from the autofocus event. But when there is this an external stylesheet, the flush doesn't work and the frames are created as soon as the stylesheet is loaded. It seems that the <script> isn't called before, is that true?

I should see what exactly prevent the flush and what could be done to have it done at the good moment.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I should see what exactly prevent the flush

How are you doing the flush?

And yes, the <script> in that testcase won't run while the stylesheet is loading.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> > I should see what exactly prevent the flush
> 
> How are you doing the flush?

It's done in |nsFocusManager::CheckIfFocusable| which is called when :Focus() is called.

> And yes, the <script> in that testcase won't run while the stylesheet is
> loading.

That explains why .focus() works but not autofocus.

What I understand now is:
The HTML5 parser send ContentAppend notifications when a new content is added to the tree. Then, the CSSFrameConstructor sets NODE_DESCENDANTS_NEED_FRAMES flag (in MaybeConstructLazily) and when the flush appends, it will check if this flag is set. Otherwise, it will not flush.
When the stylesheet is being loaded, ContentAppend is send after the flush requested by the autofocus event so the flush do nothing. The script is executed after all ContentAppend notifications so it works correctly.

The solutions I see:
1. Dispatch the autofocus event when ContentAppend is received (by PresShell, nsCSSFrameConstructor or even the HTMLElement) ;
2. Add a code in all form controls frame to run the autofocus event when they are created ;
3. Make the parser run the autofocus event when appropriate.

I guess solution 3 isn't really appropriate. For solution 1, we probably don't want to make nsGenericHTMLElement a nsIMutationObserver.
For the rest, I don't know this code enough to have a strong feeling or even be sure that why I describe is correct.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It's done in |nsFocusManager::CheckIfFocusable|

Ah, I see.

Does changing nsHtml5TreeOpExecutor::FlushPendingNotifications to pass PR_TRUE to StartLayout help things?  I'm not sure why it's passing PR_FALSE, exactly.... the old HTML sink passed PR_TRUE in that situation.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> > It's done in |nsFocusManager::CheckIfFocusable|
> 
> Ah, I see.
> 
> Does changing nsHtml5TreeOpExecutor::FlushPendingNotifications to pass PR_TRUE
> to StartLayout help things?  I'm not sure why it's passing PR_FALSE,
> exactly.... the old HTML sink passed PR_TRUE in that situation.

It's fixing it... Now, I feel useless :) (let say it's more obvious for someone who knows this code...)

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Henri, is this fix correct? I mean, is there any reason why you are passing PR_FALSE instead of PR_TRUE. According to bz (comment 15), it was PR_TRUE before.

Comment 18

[Mounir Lamouri (:mounir)]

This patch doesn't seem to decrease tp4 performance (checked on linux64).

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 499651 [details] [diff] [review]
Patch v1

(In reply to comment #17)
> Henri, is this fix correct? I mean, is there any reason why you are passing
> PR_FALSE instead of PR_TRUE. According to bz (comment 15), it was PR_TRUE
> before.

I'm not sure why I passed PR_FALSE. I probably read old code from the time before StartLayout took an argument and filled in the argument myself instead of checking how the old code had gotten updated. Or something like that.

Comment 20

[Mounir Lamouri (:mounir)]

Pushed:
https://hg.mozilla.org/mozilla-central/rev/abc78094ad64