1377447 - input autofocus looks to be broken in nightly (56)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeroen Pulles]

Attachment: autofocus.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170608105825

Steps to reproduce:

empty page with a <link rel=stylesheet href="abc"> and a <input type=text autofocus > 

browsing over filesystem: input has focus, 

browser over http:// localhost : no focus. 

If I leave out the <link rel=stylesheet href="abc">, the autofocus works. 

The href attribute does not need to resolve to a file to cause the misbehavior. See attachment. 

Or visit for similar effect: 

https://www.wufoo.com/html5/attributes/02-autofocus.html

only tested on macOS, "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:56.0) Gecko/20100101 Firefox/56.0"



Actual results:

no blue outline on the input, keyboard input goes nowhere. 


Expected results:

characters should appear in the input field when typing after load,
blue outline on the input field.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Autofocus regression

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9255719d469c99b4c11cacf6541c66e353518f24&tochange=87c1327b918d380df584858e28c23d7340c65994

Regressed by Bug 1369140

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression, tracking for 55/56. We still have some time to fix this in 55 beta.

Comment 3

[(no longer active)]

OK, so I debugged this to see why it's happening, which led to something that I don't understand.

Firstly, the bug happens because we take this branch: http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/dom/base/nsFocusManager.cpp#1592.  This happens under nsAutoFocusEvent::Run's call to Focus() trying to focus our element, so that fails.  But taking that branch makes no sense, since we *just flushed frames!*

So I looked into what happens when we flush.  mNeedsStyleFlush is true.  So is mNeedsLayoutFlush.  We get to this code: <https://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/layout/base/nsCSSFrameConstructor.cpp#7353>  Here, our document node doesn't have the NODE_DESCENDANTS_NEED_FRAMES flag set!  I would have expected us to have gotten to <https://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/layout/base/nsCSSFrameConstructor.cpp#7611> which should eventually set this flag on the document through the loop in <https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7220>.  This is my understanding of how the lazy frame construction works.

However, I realized that before the parser notifies the CSS frame constructor about the newly appended content node, we end up bailing out early here <https://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/layout/base/PresShell.cpp#4424>.  I suspect this explains why the presence of the <link> element makes some difference, it probably changes the timing of the parser getting to this point, since if the presshell has been initialized before from nsContentSink::StartLayout() and everything would work just fine.

It seems like we need to keep track of the ContentAppended/Inserted notifications arriving while mDidInitialize is false, and send them to the CSS frame constructor when the initialization happens.  Boris, Timothy, what do you guys think about that solution?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Shouldn't be necessary.

The way things work is that when PresShell::Initialize is called it constructs frames for everything that is in the DOM at that point.  See the 

  mFrameConstructor->ContentInserted(nullptr, root, nullptr, false);

call it makes.  After that that point we should listen for ContentAppended/ContentInserted notifications, set NODE_DESCENDANTS_NEED_FRAMES flags, and so forth.

The problem here is pretty simple, actually.  PresShell::Initialize can be called from a few places, but the relevant one here is nsContentSink::StartLayout.  During normal pageload this is called "normally" once we have seen <body> _and_ all stylesheets have finished loading.  In the testcase above that means it's called sometime after the autofocus attempt.  This is an attempt to avoid a flash of unstyled content.

The other path that can reach nsContentSink::StartLayout, in "ignore pending sheets" mode, so it will call PreShell::Initialize even if not all sheets are loaded, is nsHtml5TreeOpExecutor::FlushPendingNotifications.  It makes the call only if aType >= FlushType::InterruptibleLayout.  That explains the observed regression.  Similar for nsXMLContentSink::FlushPendingNotifications and HTMLContentSink::FlushPendingNotifications.

It seems like a priori not doing PresShell::Initialize on a Flush_Frames is wrong: it means we won't do frame construction.  So we should probably change those three callsites to check aType >= FlushType::Frames.  That does mean that current frame flushes that get ignored before we've got all our stylesheets will stop getting ignored.  If we want to play it really safe in terms of not introducing new flashes of unstyled content or performance issues, we could add a new flush type that sits between Frames and Layout for use from just the focus code or something.  This would be just like FlushType::Frames except it would trigger PresShell::Initialize as needed.

The other option is to defer the autofocus bits to after PresShell::Initialize happened.  That would be bug 712130.  Maybe that's the safer option here; hard to tell which is better to do for 55.  For 56, I would vaguely prefer we just fix bug 712130.

Comment 5

[(no longer active)]

I think adding a new flush type here is the safest option for now, and that should be backportable.  I will give that a shot.  Thanks for your help!

Comment 6

[(no longer active)]

Attachment: Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable

Olli, could you please review?  Boris is on vacation.

Please see comment 4 for rationale.  This is basically a long way of changing the fix for bug 1369140 to initialize the presshell if needed.

I tried to think of a good name for the new flush type but couldn't, so ended up calling it FramesForFocus because I don't expect any other consumer to want to use this, but if you can think of a nicer name, let me know, I'd be happy to change it.

Comment 7

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

Comment on attachment 8883162 [details] [diff] [review]
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable

Could you call it
EnsurePresShellInitAndFrames

Comment 8

[(no longer active)]

Sure, will do.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3edebd5f20
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable; r=smaug

Comment 10

[(no longer active)]

Comment on attachment 8883162 [details] [diff] [review]
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1369140 
[User impact if declined]: See the test case.  Some autofocus form fields may stop automatically focusing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: It just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: It isn't very risky IMO even though the patch looks really scary.  The impact of this is purely initializing the presshell before flushing in the code that was added in bug 1369140, and we used to initialize the presshell there before when we flushed layout, so it just restores that behavior.  If we decide to not take this branch the other option is a backout of that patch but the backout may end up being messy because of the test fixes that landed on top of each other over in that bug.  It's also a Speedometer performance win which I would rather not lose in 55 (but it's not the end of the world if we do, we can also just ship it in 56.  Up to release drivers.)
[Why is the change risky/not risky?]: See above (why does this form ask the same question two times?! I feel like it has never happened that I have had something useful to say to this question, besides sometimes cutting part of the answer to the previous question and pasting it here.  It's kind of annoying after a while...  I may start ignoring this question completely from now on!)
[String changes made/needed]: None.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing its own test 1377447-1.html on Windows 7 and OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6fe87c353ef03337246bbb66a35cbb827d3377

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bd3edebd5f202732d61f17b0434f426e33967c03&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=111742414&repo=mozilla-inbound

Comment 12

[(no longer active)]

Ugh.  I did a "T" try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=39cadc59706c229af151f755e6ccb7a4237699b7) and the test passed on Linux.  Sorry about that.  It seems like I should not rely on the timing of the autofocus event and use reftest-wait class on the document element explicitly.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1d781ec916
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable; r=smaug

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37fd15147624
Backout for more test failures on a CLOSED TREE

Comment 15

[(no longer active)]

Maybe this needs a needs-focus in the reftest manifest also.  Testing a patch for that on try...

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15a167e6e89
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable; r=smaug

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c15a167e6e89

Comment 18

[Julien Cristau [:jcristau]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10)
> [Why is the change risky/not risky?]: See above (why does this form ask the
> same question two times?! I feel like it has never happened that I have had
> something useful to say to this question, besides sometimes cutting part of
> the answer to the previous question and pasting it here.  It's kind of
> annoying after a while...  I may start ignoring this question completely
> from now on!)

Feel free to, if you elaborate in the previous one like you did here.  Lots of people just reply things like "no" or "maybe" or "not very" to the previous question, which is not very helpful.  (IIRC a previous version of the form had "is the change risky, and why" as a single question, and most people ignored the second part.)

Comment 19

[Marco Castelluccio [:marco]]

(In reply to Julien Cristau [:jcristau] from comment #18)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #10)
> > [Why is the change risky/not risky?]: See above (why does this form ask the
> > same question two times?! I feel like it has never happened that I have had
> > something useful to say to this question, besides sometimes cutting part of
> > the answer to the previous question and pasting it here.  It's kind of
> > annoying after a while...  I may start ignoring this question completely
> > from now on!)
> 
> Feel free to, if you elaborate in the previous one like you did here.  Lots
> of people just reply things like "no" or "maybe" or "not very" to the
> previous question, which is not very helpful.  (IIRC a previous version of
> the form had "is the change risky, and why" as a single question, and most
> people ignored the second part.)

Yes, this is exactly why we changed the question.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8883162 [details] [diff] [review]
Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable

fix an input focus regression, beta55+

Comment 21

[Carsten Book [:Tomcat]]

needs a rebased patch

Comment 22

[(no longer active)]

(In reply to Carsten Book [:Tomcat] - PTO - back July 23rd from comment #21)
> needs a rebased patch

FWIW as far as I can tell the only rebase necessary is in the reftest manifest which is trivial, I don't think it was really needed to delay the landing of this patch while I was gone for that.  :/

Comment 23

[(no longer active)]

Attachment: Add a new flush type between Frames and InterruptibleLayout to initialize layout if needed and use it from CheckIfFocusable

Rebased on beta

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33b0931bbfc0

Comment 25

[Andrei Vaida [:avaida]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: It just landed.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Ehsan's assessment on manual testing needs and the fact that this fix has automated coverage.