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

RESOLVED FIXED in mozilla2.0b9

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({html5, testcase})

Trunk
mozilla2.0b9
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
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

9 years ago
Why is no frame present? The page hasn't been laid out yet?
(Assignee)

Updated

9 years ago
Assignee: mounir.lamouri → nobody
Status: ASSIGNED → NEW

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
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

9 years ago
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?
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.
Assignee: nobody → mounir.lamouri
blocking2.0: --- → betaN+
(Assignee)

Updated

9 years ago
Summary: autofocus does not work when the page is reloaded → autofocus sometimes does not work because the element has no frame yet
(Assignee)

Updated

9 years ago
Duplicate of this bug: 600026
(Assignee)

Updated

9 years ago
Component: DOM: Core & HTML → Event Handling
QA Contact: general → events
(Assignee)

Comment 8

9 years ago
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?
(Assignee)

Comment 9

9 years ago
Posted file 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.
(Assignee)

Updated

9 years ago
Keywords: testcase
Should we move nsAutoFocusEvent to layout, so that it would be dispatched when
the frame for the form control element is created?
(Assignee)

Comment 11

8 years ago
(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...
(Assignee)

Comment 12

8 years ago
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.
> 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.
(Assignee)

Comment 14

8 years ago
(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.
> 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.
(Assignee)

Comment 16

8 years ago
(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...)
(Assignee)

Comment 17

8 years ago
Posted patch Patch v1Splinter Review
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.
Attachment #499651 - Flags: review?(hsivonen)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs-review]
(Assignee)

Updated

8 years ago
Whiteboard: [needs-review] → [needs-review][passed-try]
(Assignee)

Comment 18

8 years ago
This patch doesn't seem to decrease tp4 performance (checked on linux64).
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.
Attachment #499651 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 20

8 years ago
Pushed:
https://hg.mozilla.org/mozilla-central/rev/abc78094ad64
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs-review][passed-try]
Target Milestone: --- → mozilla2.0b9
Component: Event Handling → HTML: Parser
QA Contact: events → parser
You need to log in before you can comment on or make changes to this bug.