Closed Bug 1377447 Opened 3 years ago Closed 3 years ago

input autofocus looks to be broken in nightly (56)

Categories

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

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 + fixed

People

(Reporter: jeroen.pulles, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file 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.
[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
Blocks: 1369140
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(ehsan)
Keywords: regression
Product: Firefox → Core
Version: 56 Branch → 55 Branch
Recent regression, tracking for 55/56. We still have some time to fix this in 55 beta.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(tnikkel)
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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!
Flags: needinfo?(tnikkel)
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.
Attachment #8883162 - Flags: review?(bugs)
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
Attachment #8883162 - Flags: review?(bugs) → review+
Sure, will do.
Pushed by eakhgari@mozilla.com:
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 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.
Attachment #8883162 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
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
Backout by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37fd15147624
Backout for more test failures on a CLOSED TREE
Maybe this needs a needs-focus in the reftest manifest also.  Testing a patch for that on try...
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c15a167e6e89
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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.)
(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 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+
Attachment #8883162 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs a rebased patch
Flags: needinfo?(ehsan)
(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.  :/
Flags: needinfo?(ehsan)
Keywords: checkin-needed
(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.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.