Closed Bug 712130 Opened 13 years ago Closed 7 years ago

Autofocus should apply after the styles are applied (some pages scroll down sometimes after loading, some pages have flash of unstyled content (fouc))

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mozilla, Assigned: decltype)

References

Details

(Keywords: testcase)

Attachments

(5 files, 2 obsolete files)

testcase with a remote stylesheet
2.61 KB, text/html
Details
Bug 712130 - Defer autofocus until after frame construction.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 712130 - Add reftest to ensure the first autofocus elements gets picked.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 712130 - Add reftest in case autofocus element is seen after PresShell init.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 712130 - Simplify focus stealing prevention for autofocus.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Attached file autofocusbug.html (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0 Build ID: 20111212185108 Steps to reproduce: Using the autofocus attribute on an input field can cause a page to scroll to odd positions. It appears to be some internal race between applying styles and applying the autofocus attribute. Actual results: In a document with semantic layout, the ordering of content can be completely different to the final display position when the CSS has been applied. It appears in some cases when you use the autofocus attribute, the input is focused and page is scrolled to the position where the input field would be, but before the CSS is applied. When the CSS is then applied, the page is scrolled to the completely wrong position. The attached html page demonstrates this by dynamically inserting a style element with a setTimeout, however the same issue can occur with remotely linked stylesheets, but it's much more cache/race dependent and harder to demonstrate reliably. What you'll see is that when you load the page, it's scrolled down to the bottom, even though the styles end up moving the input to the top of the page. Expected results: The autofocus attribute should only be applied after styles have been applied, or the page scroll position should be updated again if new styles are applied.
I ran into this on FF 7.0.1 while building a page; it happened 100% of the time (might be because the machine serving the static files is slow). I had to use a JS-based solution instead of the autofocus attribute in the end.
Attachment #582953 - Attachment is obsolete: true
Component: General → Layout
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86 → All
Summary: autofocus attribute race can cause page to scroll to wrong position → autofocus attribute causes FOUC and scrolls to incorrect (unstyled) position
Version: 9 Branch → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
My testcase was reduced from a beta version of http://dict.leo.org/ with help from "bufgix".
> The autofocus attribute should only be applied after styles have been applied Yes, for the remote stylesheet case, but not guaranteeable in general for the dynamic insertion case. We should be running the autofocusevent thing off the same mechanism as script unblocking for pending stylesheets. > or the page scroll position should be updated again if new styles are applied. That's a research project on its own; there are existing bugs on it.
Component: Layout → Layout: Form Controls
This bug seems to be very consistently reproducing in Firefox 19. This was happening occasionally in Chrome and FF18, but in FF19 this now happens every time the page loads.
(In reply to Boris Zbarsky (:bz) from comment #4) > > The autofocus attribute should only be applied after styles have been applied > > Yes, for the remote stylesheet case, but not guaranteeable in general for > the dynamic insertion case. We should be running the autofocusevent thing > off the same mechanism as script unblocking for pending stylesheets. I'm afraid that focusing so let might be weird and we might have users/developers complaining that the autofocus happens too late. Unless the mechanism isn't simply delay everything until stylesheets are loaded?
Yes, the change bz and I are proposing is to delay autofocus until stylesheets are loaded, in the same cases that we currently delay scripts (and painting?).
Another alternative may be to auto focus before CSS loads, but delay scrolling the page until after CSS loads.
Whether something is focusable depends on style information. So you can't focus things without updating all style information first. And if you do that before all the CSS is loaded, you get a flash of unstyled content...
Ok, I guess we could try. We should also try to have a consistent behaviour when there are more than one autofocused element in the document. IIRC, per spec this is not allowed so we can whether focus the first or the last. Our current behaviour is to focus the last but I guess we can also focus the first as long as we keep this consistent. I'm not sure if I can free some cycles to work on that soon but if someone points me to the code handling the "stylesheet are applied, lets do some stuff", I can see if I can plug some stuff there.
Summary: autofocus attribute causes FOUC and scrolls to incorrect (unstyled) position → Autofocus should apply after the styles are applied
See nsContentSink::StartLayout. Basically we shouldn't be doing autofocus stuff if layout hasn't started and should instead set some flag that causes it to be processed when layout is actually started here.
This pure JavaScript snippet will fix the bug by refocusing the element after the onload event. // Detect if we're dealing with Firefox. if (typeof InstallTrigger !== "undefined") { var autofocus = document.querySelector("[autofocus]"); if (autofocus) { autofocus.blur(); window.onload = autofocus.focus.bind(autofocus); } }
Summary: Autofocus should apply after the styles are applied → Autofocus should apply after the styles are applied (some pages scroll down sometimes after loading)
See Also: → 60307
So I was looking at this again, and I just don't understand the current behavior of nsAutoFocusEvent. It looks like it skips focusing anything if topDoc is fully loaded. But that means that the runnable is racing with the end of load, no? And if it loses the race, no autofocus... That seems like an odd design behavior. Why do we do that? Olli, you reviewed that code; do you recall?
Flags: needinfo?(bugs)
Looking at bug 546995 it seems that the reason is that Jonas suggested that we shouldn't autofocus something if an element was added back to the DOM after page load because it might not be on purpose. There might be additional reasons mentioned in the bug. Though, this is where the nsAutoFocusEvent comes from it seems.
The behavior does indeed look racy. We could block/unblock onload in nsAutoFocusEvent. (block when dispatching, unblock in Run()), and dispatch nsAutoFocusEvent either in BindToTree (if layout has started) or when SetMayStartLayout(true) is called. Does that sound ok to you bz?
Flags: needinfo?(bugs) → needinfo?(bzbarsky)
Would need to block onload of the top... Hmm, how would that work. Need to block onload of the top earlier, when nsAutoFocusEvent is created, and unblock perhaps then in dtor.
Seems like we should either do that or check the toplevel load state at nsAutoFocusEvent creation time.
Flags: needinfo?(bzbarsky)
oh, sure, and then block/unblock onload. Need to still block, otherwise the runnable might run when load has already fired.
Why is that a problem? We'd just remove that check from the Run() method, if the goal was to prevent the issue of comment 17. Seems like if we add something before onload and then onload fires we should still autofocus, but that need not block onload...
I thought you wanted to remove some racyness, but perhaps not that racyness then.
Ah, I wanted to remove the raciness where whether something gets focused or not depends not on when we post the nsAutoFocusEvent (which happens deterministically) but on the ordering of that event firing and toplevel onload. So we could remove that raciness by ensuring that the event always blocks onload, or by moving the check to event posting, not firing. That would make it simpler to post the event later than we do now, in particular from SetMayStartLayout(true), without running into issues with it firing after onload causing lack of autofocus. It might still fire after onload, but would focus anyway.
Btw, while fixing this I think we should update the implementation to follow the spec - looks like the spec has changed quite a bit since the time autofocus was implemented. This is in my todo list, but feel free to take it.
Summary: Autofocus should apply after the styles are applied (some pages scroll down sometimes after loading) → Autofocus should apply after the styles are applied (some pages scroll down sometimes after loading, some pages have flash of unstyled content (fouc))
Confirmed this issue with 44.0.2 Because our nav is very tall un-styled a user is dropped at the end of the page once styles are applied. While working on this I recommend that you also check to make sure we only scroll when needed. I have heard of some issues where the autofocused field is already above the fold but scrolls anyways bringing elements above out of view which is not the best experience for the user.
(In reply to Jesse Ruderman from comment #2) > Created attachment 714985 [details] > testcase with a remote stylesheet Firefox 47.0 scrolls to the input, so that seems to work as intended. But I found another page with strange scroll behaviour. Steps to reproduce: 1. Disable JavaScript. 2. Clear cache. 3. Go to http://www.duden.de/rechtschreibung/Test Result: The input on the top of the page is focused, but the page is scrolled to the bottom.
I can reproduce the problem on another Website with ease with the current Version of Firefox for Ubuntu (50.1.0) When you open the startpage of the Meta-Searchengine MetaGer: 1. Open Firefox 2. Shrink the height of the Firefox Window (Because the webpage needs to be scrollable) 3. Go to https://metager.de/en 4. You will see that the firefox scrolls all the way to the bottom of the page even though the Input for the search (on which the autofocus attribute is) will then no longer be visible. It seems that Firefox scrolls way to far down to the end of the page. The search box would be visible with no scrolling needed and every other browser that I tested doesn't scroll down.
See also the discussion in bug 1377447 for some context. We might want to undo that change when we fix this bug.
We definitely want to undo that change when we fix this bug.
I ran into this bug as well. (Version 55.0.2) And then found this workaround: https://stackoverflow.com/questions/18943276/html-5-autofocus-messes-up-css-loading
I ran into the same issue on both 55.0.3 and 57 Beta.
See Also: → 1404468
We've got reports on this too on Wikipedia. We can reproduce on FF 57 on Mac OS X, test it with https://de.wikipedia.org/wiki/Spezial:ISBN-Suche Timo has some info here how he debugged it: https://phabricator.wikimedia.org/T181877#3811469
We have this bug in several ecommerce systems when the user can enter his ordering data. The suggested java script workarounds do not work in firefox quantum.
Can confirm that the workaround mentioned by Leon works in Firefox 57.0: https://stackoverflow.com/a/18945951/104976
Comment on attachment 8940874 [details] Bug 712130 - Back out new flush type introduced in c15a167e6e89 for Bug 1377447. Doesn't this regress Speedometer score a lot?
(In reply to Olli Pettay [:smaug] from comment #43) > Doesn't this regress Speedometer score a lot? Not as far as I can tell.
Comment on attachment 8940874 [details] Bug 712130 - Back out new flush type introduced in c15a167e6e89 for Bug 1377447. https://reviewboard.mozilla.org/r/211120/#review217166 I think we don't need to back this out. In theory some script might call focus() before document is loaded.
Attachment #8940874 - Flags: review?(bugs)
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217176 This looks pretty reasonable, but some minor fixes and explanations needed. ::: dom/base/nsDocument.h:1328 (Diff revision 1) > RefPtr<nsContentList> mImageMaps; > > + // NOTE: nsGenericHTMLFormElement is saved as a nsGenericHTMLElement > + // because AddRef/Release are ambiguous with nsGenericHTMLFormElement > + // and Focus() is declared (and defined) in nsGenericHTMLElement class. > + RefPtr<nsGenericHTMLElement> mAutoFocusElement; This is a strong reference, so please add this member variable to the cycle collection. Look at NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(nsDocument) and NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDocument) in nsDocument.cpp (just in case nsDocument::TriggerAutoFocus() doesn't get triggered in some error cases.) Or, it might be even safer to use nsWeakPtr ::: dom/base/nsDocument.cpp:9992 (Diff revision 1) > + : mozilla::Runnable("nsAutoFocusEvent") > + , mElement(Move(aElement)) > + { > + } > + > + NS_IMETHOD Run() override { I know you're moving this code, but want to fix coding style nit: { after method definition should be on its own line. ::: dom/base/nsDocument.cpp:10032 (Diff revision 1) > + // and Focus() is declared (and defined) in nsGenericHTMLElement class. > + RefPtr<nsGenericHTMLElement> mElement; > +}; > + > +void > +nsDocument::SetAutoFocusElement(nsGenericHTMLFormElement* autoFocusElement) Nit, arguments should be named aName, so aAutoFocusElement ::: layout/base/nsDocumentViewer.cpp:796 (Diff revision 1) > } > } > > if (aDoInitialReflow && mDocument) { > mDocument->ScrollToRef(); > + mDocument->TriggerAutoFocus(); Why we trigger autofocus here, but not in nsContentSink::ScrollToRef() ?
Attachment #8940873 - Flags: review?(bugs) → review-
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217176 > This is a strong reference, so please add this member variable to the cycle collection. Look at > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(nsDocument) and NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDocument) in nsDocument.cpp > (just in case nsDocument::TriggerAutoFocus() doesn't get triggered in some error cases.) > > Or, it might be even safer to use nsWeakPtr I went with nsWeakPtr, as there is no need to keep the element alive. > Why we trigger autofocus here, but not in nsContentSink::ScrollToRef() ? Good point. In any situation where ScrollToRef is called, we probably also want to trigger autofocus. nsContentSink::ScrollToRef seems to be concerned exclusively with URI anchors, according its comment. If we repurpose it to do both, it would have to be renamed to something more general. Alternatively, we could also combine TriggerAutoFocus and ScrollToRef at the nsIDocument level. That would save one virtual call and also allow us to combine mScrolledToRefAlready with mAutoFocusFired.
Attachment #8940874 - Attachment is obsolete: true
Attachment #8940874 - Flags: review?(bzbarsky)
So do other browsers have the behavior the patch gives (I wonder how to test that)? Since it is different to what the spec says. The spec "When an element with the autofocus attribute specified is inserted into a document, user agents should run the following steps:" ... "Queue a task that runs the focusing steps for the element." The patch postpones that task possibly quite a bit. If other browsers do something closer to what the patch does, we need to get the spec changed. (https://whatwg.org/newbug)
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217278 Thank you for working on this! I'm sorry the code involved is such a complicated mess. :( ::: dom/base/nsDocument.cpp:10006 (Diff revision 2) > + nsPIDOMWindowOuter* window = document->GetWindow(); > + if (!window) { > + return NS_OK; > + } > + > + // Trying to found the top window (equivalent to window.top). s/found/find/ (I know this was preexisting). ::: dom/base/nsDocument.cpp:10032 (Diff revision 2) > +}; > + > +void > +nsDocument::SetAutoFocusElement(Element* aAutoFocusElement) > +{ > + if (!mAutoFocusFired) { This doesn't seem right, and we should be able to write a testcase accordingly. Specifically, consider what happens if TriggerAutoFocus() is called for the first time (e.g. due to presshell init via the document viewer) before we've seen an element with the autofocus attribute. In that case, we will fail to autofocus: in the first TriggerAutoFocus we will no-op because mAutoFocusElement is null, and in later SetAutoFocusElement calls we'll no-op because mAutoFocusFired is true. ::: dom/html/nsGenericHTMLElement.cpp:1832 (Diff revision 2) > // specified and the element accept the autofocus attribute. In addition, > // the document should not be already loaded and the "browser.autofocus" > // preference should be 'true'. > if (IsAutofocusable() && HasAttr(kNameSpaceID_None, nsGkAtoms::autofocus) && > - nsContentUtils::AutoFocusEnabled()) { > - nsCOMPtr<nsIRunnable> event = new nsAutoFocusEvent(this); > + nsContentUtils::AutoFocusEnabled() && aDocument) { > + aDocument->SetAutoFocusElement(this); Doesn't this change behavior if there are multiple elements with the "autofocus" attr set? Before this patch, each would queue an event, the first event would focus an element, and the other events would no-op, focusing the _first_ autofocusable thing After the patch, it looks like the last call to SetAutoFocusElement would win in terms of what gets stored on the document and then the _last_ autofocusable thing would get focused. Please add a testcase? ::: layout/base/nsDocumentViewer.cpp:796 (Diff revision 2) > } > } > > if (aDoInitialReflow && mDocument) { > mDocument->ScrollToRef(); > + mDocument->TriggerAutoFocus(); Hmm. So if we land in InitPresentationStuff with aDoInitialReflow false (e.g. via Show() with mDocument->MayStartLayout() false), and then start layout via nsContentSink::StartLayout, then we won't trigger autofocus until LoadComplete, right? That seems a bit unfortunate; LoadComplete can be arbitrarily delayed by ads and whatnot. I think doing what Olli suggests and trying autofocus any time ScrollToRef happens would probably help with this issue a bit. But there are cases where we StartLayout (e.g. if we hit `<body>` and there are not stylesheet loads pending) but don't ScrollToRef... Conceptually, it seems to me like we should try to autofocus the first time both of "presshell is initialized" and "element with autofocus attr is in the DOM" are true, or something along those lines.
Attachment #8940873 - Flags: review?(bzbarsky) → review-
> So do other browsers have the behavior the patch gives (I wonder how to test that)? I just looked at the Chromium source code and they queue the task towards the end of layout/style calculation. There's also a comment in there pointing out that it differs from the standard: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/forms/HTMLFormControlElement.cpp?l=267&rcl=5e101805e7f66d878389df053a4ce8a186e98d00
> Doesn't this regress Speedometer score a lot? It wouldn't, because it's backing out the fix for bug 1377447, not the fix for bug 1369140. > In theory some script might call focus() before document is loaded. More precisely before presshell init. The question is what should happen in that case. I think before bug 1377447 was fixed (but after bug 1369140), the focus() call just failed to focus things in that situation. I guess that's not very good; given that, comment 32 was just wrong.
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217280
Attachment #8940873 - Flags: review?(bugs)
(In reply to decltype from comment #52) > > So do other browsers have the behavior the patch gives (I wonder how to test that)? > > I just looked at the Chromium source code and they queue the task towards > the end of layout/style calculation. There's also a comment in there > pointing out that it differs from the standard: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/ > forms/HTMLFormControlElement. > cpp?l=267&rcl=5e101805e7f66d878389df053a4ce8a186e98d00 FWIW, just a minor nit, that happens after style recalc, but before layout. That's effectively our frame construction code.
Webkit seems to have somewhat similar behavior to Chrome. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/html/HTMLFormControlElement.cpp#L236 Want to file a spec bug?
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217278 > This doesn't seem right, and we should be able to write a testcase accordingly. Specifically, consider what happens if TriggerAutoFocus() is called for the first time (e.g. due to presshell init via the document viewer) before we've seen an element with the autofocus attribute. In that case, we will fail to autofocus: in the first TriggerAutoFocus we will no-op because mAutoFocusElement is null, and in later SetAutoFocusElement calls we'll no-op because mAutoFocusFired is true. Adjusted to only set mAutoFocusFired when we actually dispatch an nsAutoFocusEvent and added a reftest. > Doesn't this change behavior if there are multiple elements with the "autofocus" attr set? Before this patch, each would queue an event, the first event would focus an element, and the other events would no-op, focusing the _first_ autofocusable thing After the patch, it looks like the last call to SetAutoFocusElement would win in terms of what gets stored on the document and then the _last_ autofocusable thing would get focused. Please add a testcase? Right, we should keep the old behavior. Added a reftest. > Hmm. So if we land in InitPresentationStuff with aDoInitialReflow false (e.g. via Show() with mDocument->MayStartLayout() false), and then start layout via nsContentSink::StartLayout, then we won't trigger autofocus until LoadComplete, right? That seems a bit unfortunate; LoadComplete can be arbitrarily delayed by ads and whatnot. > > I think doing what Olli suggests and trying autofocus any time ScrollToRef happens would probably help with this issue a bit. But there are cases where we StartLayout (e.g. if we hit `<body>` and there are not stylesheet loads pending) but don't ScrollToRef... > > Conceptually, it seems to me like we should try to autofocus the first time both of "presshell is initialized" and "element with autofocus attr is in the DOM" are true, or something along those lines. I added a call to TriggerAutoFocus during PresShell initialization after the frames are constructed, as well as in SetAutoFocusElement. TriggerAutoFocus now checks for both conditions.
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217176 > Good point. In any situation where ScrollToRef is called, we probably also want to trigger autofocus. > > nsContentSink::ScrollToRef seems to be concerned exclusively with URI anchors, according its comment. If we repurpose it to do both, it would have to be renamed to something more general. > > Alternatively, we could also combine TriggerAutoFocus and ScrollToRef at the nsIDocument level. That would save one virtual call and also allow us to combine mScrolledToRefAlready with mAutoFocusFired. As Boris suggested, we now trigger autofocus during PresShell initialization as well as when the first autofocus element is seen (earliest point when both are satisfied).
(In reply to Olli Pettay [:smaug] from comment #56) > Want to file a spec bug? Since the other browsers also do not exactly follow the spec, we aren't likely to break anything by applying this interim patch. As to whether the spec should be changed: We need to think about whether setting focus really requires style calculation. Certain styles like 'visibility: hidden' do affect focusability but I haven't found any place in the spec where this is rigorously defined. We might be okay with handing out focus at first and then blur()-ing once the styles get set. It looks like the main reasons we force a PresShell init before focusing an element are: - To check whether we are in a print preview - Handling of -moz-user-focus style, which ended up not getting standardized - Doing some frame visibility checks, including the visibility style There is currently no way to force style calculation without also triggering layout and unsuppressing painting. If we decide that styles do need to be flushed before handing out focus, these should be separated out. That way we can get rid of the FOUC without deferring autofocus.
Comment on attachment 8941543 [details] Bug 712130 - Add reftest to ensure the first autofocus elements gets picked. https://reviewboard.mozilla.org/r/211804/#review217606 Does this fail without the checks we added to fix this? I ask because this test doesn't have a stylesheet, so inits the presshell as soon as it sees the `<body>` (which is when it sees the first `<input>`). It might be more robust to have a stylesheet load in there to make it more likely that we parse both of the inputs before trying to autofocus.
Attachment #8941543 - Flags: review?(bzbarsky) → review+
Comment on attachment 8941544 [details] Bug 712130 - Add reftest in case autofocus element is seen after PresShell init. https://reviewboard.mozilla.org/r/211806/#review217608
Attachment #8941544 - Flags: review?(bzbarsky) → review+
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review217610 r=me. This looks great, thank you!
Attachment #8940873 - Flags: review?(bzbarsky) → review+
Assignee: nobody → mozilla
Comment on attachment 8941543 [details] Bug 712130 - Add reftest to ensure the first autofocus elements gets picked. https://reviewboard.mozilla.org/r/211804/#review217606 It would have failed with the previous version, but you are right, adding a stylesheet makes it more robust.
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. bz' review is enough
Attachment #8940873 - Flags: review?(bugs)
I will submit an updated patch which leaves autofocus disabled after page load, otherwise dom/html/reftests/autofocus/autofocus-after-load.html breaks.
Keywords: checkin-needed
We still need to get rid of the race between nsAutoFocusEvent and the page load (see https://bugzilla.mozilla.org/show_bug.cgi?id=712130#c16). The question is whether we want to do most of the checks from nsAutoFocusEvent::Run during event posting or to allow autofocus after load.
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. Since bz reviewed other patches here, I think his review is enough in this case too.
Attachment #8940873 - Flags: review?(bugs)
Comment on attachment 8942010 [details] Bug 712130 - Simplify focus stealing prevention for autofocus. https://reviewboard.mozilla.org/r/212212/#review218052 ::: dom/base/nsDocument.cpp:9997 (Diff revision 1) > - } > - > - nsIDocument* document = mElement->OwnerDoc(); > - > // Don't steal focus from the user. > if (mRootWindow->GetFocusedNode()) { So the point is that anything that sets the focused content on the focus manager will also set the focused node on some windows, and if that focused content is in our doc, those windows will include mRootWindow? That looks about right, yeah... Might be good to just say that explicitly in the commit message.
Attachment #8942010 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ba6c2fff9fd5 Add reftest to ensure the first autofocus elements gets picked. r=bz https://hg.mozilla.org/integration/autoland/rev/a3cffbfc8395 Add reftest in case autofocus element is seen after PresShell init. r=bz https://hg.mozilla.org/integration/autoland/rev/3774e90777a7 Defer autofocus until after frame construction. r=bz https://hg.mozilla.org/integration/autoland/rev/e7738f07edae Simplify focus stealing prevention for autofocus. r=bz
Keywords: checkin-needed
FYI. I had the same behavior using firefox >= v.56. For example following page was firstly always rendered unstyled, without css, hereafter with latency up to 1 second, the page was rendered correctly: [https://de.wikipedia.org/wiki/Artikel](https://de.wikipedia.org/wiki/Artikel) Just to exclude the case that something profile-related causes the issue, I've tried firefox with new profile (`firefox -p`)... And it looked alright! So back to my default profile, and tried to find the evil-doers... E. g. also with disabling of some add-on's, etc. In my case it was the "Ghostery" extension. After disabling it - no render-latencies (unstyled content) anymore. But I don't know (not tested) whether this add-on causes it alone (or possibly just conflicts with some other).
decltype, are you still working on this? sebres, you should report that as a bug to the Ghostery extension. There can be various reasons for flashes of unstyled content. This bug is about the autofocus case, but Ghostery could be causing such flashes easily by (mis-)using certain APIs at certain times.
> you should report that as a bug to the Ghostery extension I've not been planning to report in to Ghostery (just no time, 'cause persistent time-pressed)... You can do it, if you want. I have wanted just to tell here, why the issue could occur.
I sent Ghostery a bug report.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #88) > decltype, are you still working on this? Yep, sorry for the delay. The previous patch contained an invalid assumption that the non-scripted top window is the same as the root window, which is not the case on Android. I've already updated those on reviewboard and the tests are now passing. However, the new autofocus tests I added appear to be flaky on Android (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=253880e55c5c&selectedJob=156072286), likely due to Skia rendering. I am planning to prefix those with 'fuzzy-if(skiaContent,1,3)' since that's also the threshold used for single focused inputs in dom/html/reftests/autofocus/reftest.list.
> Yep, sorry for the delay. All good. Not trying to rush you; just wanted to make sure we didn't need someone else to pick things up.
Changes since the review: https://reviewboard.mozilla.org/r/211118/diff/3-9/ This removes the race between nsAutoFocusEvent running and page load. I also added a check for 'autoFocusElement->OwnerDoc() == this' when autofocus gets triggered, in case the element somehow got moved to another document.
Flags: needinfo?(mozilla) → needinfo?(bzbarsky)
Comment on attachment 8941543 [details] Bug 712130 - Add reftest to ensure the first autofocus elements gets picked. https://reviewboard.mozilla.org/r/211804/#review223432
Comment on attachment 8941544 [details] Bug 712130 - Add reftest in case autofocus element is seen after PresShell init. https://reviewboard.mozilla.org/r/211806/#review223434
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review223436 ::: dom/base/nsDocument.cpp:10063 (Diff revisions 3 - 9) > + nsCOMPtr<nsPIDOMWindowOuter> window = GetWindow(); > + if (!window) { > + return; > + } > + > + // Trying to find the top window (equivalent to window.top). We should document somewhere why we're caching the top window in the event. It's not clear at all. ::: dom/base/nsDocument.cpp:10071 (Diff revisions 3 - 9) > + } > + > + // NOTE: This may be removed in the future since the spec technically > + // allows autofocus after load. > + nsCOMPtr<nsIDocument> topDoc = window->GetExtantDoc(); > + if (topDoc && topDoc->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE) { Why is this check being done on topDoc? This basically disables autofocus on subframe navigations, right? That doesn't seem like what we want.
Attachment #8940873 - Flags: review+
Comment on attachment 8942010 [details] Bug 712130 - Simplify focus stealing prevention for autofocus. https://reviewboard.mozilla.org/r/212212/#review223438 ::: commit-message-9f925:3 (Diff revision 5) > +Bug 712130 - Simplify focus stealing prevention for autofocus. r?bz > + > +Remove redundant check already implied by earlier condition: If the focus This is only true if the top window of mElement is still mTopWindow.... That's why I'm wondering why we're caching mTopWindow now instead of computing it at this point.
Attachment #8942010 - Flags: review+
Flags: needinfo?(bzbarsky)
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review223436 > We should document somewhere why we're caching the top window in the event. It's not clear at all. You're right. I cached the window because we've already done the readystate check on it before dispatching the event, so we should make sure to use that window and not a potentially unrelated one. But then, we are still missing a check inside nsAutoFocusEvent::Run to ensure the cached window is actually still the top-level window of the element. I'm not entirely if that's enough, since the [specification](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofocusing-a-form-control:-the-autofocus-attribute) seems to inherently contain a race condition between steps 1-8 and step 9. For example, there may already user interaction tasks queued up that correspond to the user clicking and starting to type in another form control. A script, or the user, could also have changed the top-level browsing context's active document by navigating away. In general any check done before dispatching the event may already have been invalidated by the time the event runs. > Why is this check being done on topDoc? This basically disables autofocus on subframe navigations, right? That doesn't seem like what we want. This is behavior was already present before the patch. I think the check is being done on the topDoc in keeping with the spirit of the specification. Notably autofocus may only be triggered once among all document with the same "top-level browsing context's active document." Of course, the specification says nothing about the document's readystate, so as the comment above explains this is technically out of spec.
Comment on attachment 8942010 [details] Bug 712130 - Simplify focus stealing prevention for autofocus. https://reviewboard.mozilla.org/r/212212/#review223438 > This is only true if the top window of mElement is still mTopWindow.... That's why I'm wondering why we're caching mTopWindow now instead of computing it at this point. We should definitely check that it's still the element's top window, but we also wouldn't want to give the element focus if it's now in an entirely different window than at event dispatch time. So maybe we should cache and also recompute at this point, bailing out if anything changed?
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review223436 > You're right. I cached the window because we've already done the readystate check on it before dispatching the event, so we should make sure to use that window and not a potentially unrelated one. But then, we are still missing a check inside nsAutoFocusEvent::Run to ensure the cached window is actually still the top-level window of the element. > > I'm not entirely if that's enough, since the [specification](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofocusing-a-form-control:-the-autofocus-attribute) seems to inherently contain a race condition between steps 1-8 and step 9. For example, there may already user interaction tasks queued up that correspond to the user clicking and starting to type in another form control. A script, or the user, could also have changed the top-level browsing context's active document by navigating away. In general any check done before dispatching the event may already have been invalidated by the time the event runs. I agree the spec has a race condition. I just filed https://github.com/whatwg/html/issues/3467 on that. For now, I think we should just make sure we're still in the same window and skip autofocus if not. > This is behavior was already present before the patch. > > I think the check is being done on the topDoc in keeping with the spirit of the specification. Notably autofocus may only be triggered once among all document with the same "top-level browsing context's active document." Of course, the specification says nothing about the document's readystate, so as the comment above explains this is technically out of spec. Ah, ok. Thank you for explaining...
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review223436 > I agree the spec has a race condition. I just filed https://github.com/whatwg/html/issues/3467 on that. > > For now, I think we should just make sure we're still in the same window and skip autofocus if not. Thanks for filing the spec bug. I've updated the patch as you suggested.
Comment on attachment 8942010 [details] Bug 712130 - Simplify focus stealing prevention for autofocus. https://reviewboard.mozilla.org/r/212212/#review223438 > We should definitely check that it's still the element's top window, but we also wouldn't want to give the element focus if it's now in an entirely different window than at event dispatch time. So maybe we should cache and also recompute at this point, bailing out if anything changed? Resolved by the changes to the previous commit.
Comment on attachment 8940873 [details] Bug 712130 - Defer autofocus until after frame construction. https://reviewboard.mozilla.org/r/211118/#review226256
Attachment #8940873 - Flags: review?(bzbarsky) → review+
Comment on attachment 8942010 [details] Bug 712130 - Simplify focus stealing prevention for autofocus. https://reviewboard.mozilla.org/r/212212/#review226258
Attachment #8942010 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/302d513bcb40 Add reftest to ensure the first autofocus elements gets picked. r=bz https://hg.mozilla.org/integration/autoland/rev/0208069d0121 Add reftest in case autofocus element is seen after PresShell init. r=bz https://hg.mozilla.org/integration/autoland/rev/a87c09c4434a Defer autofocus until after frame construction. r=bz https://hg.mozilla.org/integration/autoland/rev/dc9ba6649af7 Simplify focus stealing prevention for autofocus. r=bz
Keywords: checkin-needed
Depends on: 1463563
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: