Mike Conley (:mconley) (:⚙️) Reporter
	Description • 7 years ago

Element.focus() appears to be one of those things that, without fail, causes a sync reflow.

Example:

http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/toolkit/content/widgets/textbox.xml#228-237

Here, the textbox binding is calling focus() on the inputField on a mousedown event handler. It's possible that there are other mousedown event handlers that will fire in this same tick, but we're going to jank those in order to do what might be a full layout recalculation.

Hey Enn, out of curiosity, do you know why it is necessary to flush layout on focus?

	Neil Deakin
	Comment 1 • 7 years ago

It probably wasn't necessary in some/most cases, but removing the flush entirely will cause many test failures and quite likely many compatibility regressions.

The page at https://gist.github.com/paulirish/5d52fb081b3570c81e3a suggests that Chrome does multiple flushes when focus() is called.

You might consider whether the specific case of calling focus() from script can be changed, but I suspect that removing it will cause some subtle issues.

For reference, the flush was added by bug 305840.

	Neil Deakin
	Comment 2 • 7 years ago

Removing the flush entirely causes many test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb0495997d5c2f8117c07a4d1a721bd31039540

	Florian Quèze [:florian]
	Comment 3 • 7 years ago

If removing the flush isn't possible, could we make the flush lazy instead? (eg. do it when some code calls the document.activeElement getter?)

	Neil Deakin
	Comment 4 • 7 years ago

The flush occurs when determining whether the element is focusable or not.

For example, a page that redirects focus to a just created element on mousedown. I suspect there is some editing tool that does this.

The other bug (792297) removed one of the flushes but smaug didn't think it was worth it; perhaps that at least could be revisited.

	Florian Quèze [:florian]
	Comment 5 • 7 years ago

(In reply to Neil Deakin from comment #4)
> The flush occurs when determining whether the element is focusable or not.

Can we skip this test for some common cases? Eg. focusing the location bar should always be possible, especially when it has just been clicked. Focusing a tab is probably always possible too. Or am I missing something?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

Why doesn't https://bugzilla.mozilla.org/show_bug.cgi?id=792297#c11 still apply?


Curious, could we move the JS calling .focus() to happen in an animation frame callback? We're going to flush right after that anyhow, so the flush might not be so bad, if it makes the flush happening afterwards no-op.

	Florian Quèze [:florian]
	Comment 7 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #6)
> Why doesn't https://bugzilla.mozilla.org/show_bug.cgi?id=792297#c11 still
> apply?
> 
> 
> Curious, could we move the JS calling .focus() to happen in an animation
> frame callback?

Possibly. We filed this bug and started this discussion because in bug 1349211 comment 4 I observed that some of the time we spend when opening tabs is sync flushes when moving the focus at least twice.

Before we started digging into that code to ensure we set the focus only once (which we should probably do anyway for that case), we wanted to check if there was something that could be done on the platform side to make .focus() calls cheap enough that we wouldn't need to care if the focus is moved more than once by JS code.

The specific case of setting the focus twice may not be the only place where we have this problem, so if possible it would be nicer to fix the whole category of problems rather than just one specific occurrence.

	(no longer active)
	Comment 8 • 7 years ago

(In reply to Neil Deakin from comment #4)
> The flush occurs when determining whether the element is focusable or not.

Why do we need to flush layout for that and not frames?

	Neil Deakin
	Comment 9 • 7 years ago

Only a few failures for flushing frames instead; could be possible to fix them up:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f870b525670f96b23aaee869d05bf622beb4b63

	Dão Gottwald [:dao]
	Comment 11 • 7 years ago

(In reply to Mike Conley (:mconley) from comment #10)
> Bug 1369140 is on inbound, and has landed the approach that causes us to
> flush _frames_ (and not styles or layout!) when switching focus.

frames and style is the same, I believe. See http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/base/FlushType.h#28-29

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 12 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to Mike Conley (:mconley) from comment #10)
> > Bug 1369140 is on inbound, and has landed the approach that causes us to
> > flush _frames_ (and not styles or layout!) when switching focus.
> 
> frames and style is the same, I believe. See
> http://searchfox.org/mozilla-central/rev/
> 1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/base/FlushType.h#28-29

Ah, TIL. Thank you. I'll mark my comment as obsolete.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 13 • 7 years ago

In bug 1369140, ehsan made focus switching only flush styles, and not layout.

I'm going to work my way through the dependencies here, but I'm pretty certain these are all fixed now.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 14 • 7 years ago

Apparently not all are fixed. I'll file a new bug for them, transfer dependencies, and close this one out.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 15 • 7 years ago

Rewiring remaining bugs, and marking them blocking bug 1371371. We no longer guarantee a layout flush during focus switches, so I think this bug is done.