1262946 - Don't focus the initial browser in a new window until it has been painted

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

For e10s, doing the focus switch before painting / composing the web content will slow the presenting of the web content to the user due to sync IME messages that we seem to be unable to avoid.

If we wait until the paint has occurred to do the focus change, at least the user will see some content painted, which is good.

We want to make sure, however, that we don't switch focus if the activeElement in the document changes while we're waiting for content to paint (ie, the user changed focus to something else). Stealing focus is no-bueno.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1262946 - Don't focus the initial browser of a new window until it has painted. r?Gijs

This is in order to optimize the critical path (the presenting of content to the user).
If we don't wait until the content has been presented for the tab switch, then we run
the risk of causing the content to send sync IPC messages for IME up to the parent,
which slows down the rendering of the content.

Review commit: https://reviewboard.mozilla.org/r/45063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45063/

Comment 2

[:Gijs (he/him)]

Something that's a bit hard for me to evaluate here is what happens if the browser tries to load something in this new window that takes a long time to respond? Does MozAfterPaint fire for the initial about:blank load or shortly thereafter? Or not?

What I'm getting at is that stealing focus after a significant amount of time has passed would be more likely to go wrong. What if the user has created a new tab and done something else there since then, for instance? Are we going to try to focus the now-hidden-in-a-background-tab document?

Mike, can you clarify? Maybe we should have a timeout on wallclock time? That feels a bit ugly, but might contain risk?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #2)
> Something that's a bit hard for me to evaluate here is what happens if the
> browser tries to load something in this new window that takes a long time to
> respond? Does MozAfterPaint fire for the initial about:blank load or shortly
> thereafter? Or not?

What I've learned is that when loading a document, we have a heuristic that determines when we have enough information to paint something sensible for the user. There's that, and a timeout. For the former, as soon as we have enough information, we paint, and next vsync, we composite, and MozAfterPaint fires. For the latter case, where it's taking a while to get enough document information, we'll hit the timeout, and then we'll paint to show something to the user. At next composite, MozAfterPaint fires.

And if the page is taking forever to load, we'll eventually fire MozAfterPaint for the about:blank that gets loaded instead.

> 
> What I'm getting at is that stealing focus after a significant amount of
> time has passed would be more likely to go wrong. What if the user has
> created a new tab and done something else there since then, for instance?
> Are we going to try to focus the now-hidden-in-a-background-tab document?

If that occurs, I would expect that the activeElement on the document will have changed. Unless I'm very much mistaken, the only way we're "stealing focus" is if the user hasn't moved focus, so we didn't really steal it - we just waited until later to move it. If the user opens a new tab, focuses the URL bar, etc, I think we'll end up just skipping the call to focus the selected browser.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8739140 [details]
MozReview Request: Bug 1262946 - Don't focus the initial browser of a new window until it has painted. r?Gijs

https://reviewboard.mozilla.org/r/45063/#review41611

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> > What I'm getting at is that stealing focus after a significant amount of
> > time has passed would be more likely to go wrong. What if the user has
> > created a new tab and done something else there since then, for instance?
> > Are we going to try to focus the now-hidden-in-a-background-tab document?
> 
> If that occurs, I would expect that the activeElement on the document will
> have changed. Unless I'm very much mistaken, the only way we're "stealing
> focus" is if the user hasn't moved focus, so we didn't really steal it - we
> just waited until later to move it. If the user opens a new tab, focuses the
> URL bar, etc, I think we'll end up just skipping the call to focus the
> selected browser.

OK. Can we make a test for this? That would make me feel better... :-)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

Review commit: https://reviewboard.mozilla.org/r/45129/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45129/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739140 [details]
MozReview Request: Bug 1262946 - Don't focus the initial browser of a new window until it has painted. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45063/diff/1-2/

Comment 7

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/45063/#review41633

Just a heads up that I changed this one - I had to move the event handler in content to tab-content.js, since I forgot that we'll want non-e10s to do this as well.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/45063/#review41633

Also, I switched to using commandDispatcher.focusedElement instead, since this has less to do with "[the element that will get keystroke events](https://developer.mozilla.org/en-US/docs/Web/API/Document/activeElement)" and more about focus in general.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739229 [details]
MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45129/diff/1-2/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8739229 [details]
MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

https://reviewboard.mozilla.org/r/45129/#review41677

This test is failing on Linux, but the change is also causing browser/base/content/test/general/browser_bug495058.js to fail across the board in e10s mode. :-(

::: browser/base/content/test/general/browser_newwindow_focus.js:31
(Diff revision 2)
> + * Test that when a new window is opened from content that focus
> + * the initial browser in that new window once that window has
> + * finished painting.

This sentence is suffering from trying to say 2 things at once, I think? Maybe:

Test that when a new window is opened from content, focus moves to the initial browser in that window once the window has finished painting.

::: browser/base/content/test/general/browser_newwindow_focus.js:74
(Diff revision 2)
> +    yield BrowserTestUtils.synthesizeMouseAtCenter("#target", {}, browser);
> +    let newWin = yield newWinPromise;
> +
> +    // Because we're switching focus, we shouldn't steal it once
> +    // content paints.
> +    newWin.gURLBar.focus();

I don't think you have a guarantee that newWin.gURLBar exists here. The domWindowOpened promise will resolve very quickly - even before the content in the new window is loaded, in some cases.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/45129/#review41677

Investigating.

> This sentence is suffering from trying to say 2 things at once, I think? Maybe:
> 
> Test that when a new window is opened from content, focus moves to the initial browser in that window once the window has finished painting.

Yeah, that's better, thanks.

> I don't think you have a guarantee that newWin.gURLBar exists here. The domWindowOpened promise will resolve very quickly - even before the content in the new window is loaded, in some cases.

> even before the content in the new window is loaded, in some cases.

What are those cases? From my local testing, it looks like the DOMContentLoaded / load events are fired in the new window before domwindowopened gets sent around.

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> What are those cases? From my local testing, it looks like the
> DOMContentLoaded / load events are fired in the new window before
> domwindowopened gets sent around.

That's surprising to me. If I run this:

foo = (s,t,d) => { s.QueryInterface(Ci.nsIDOMWindow); console.log(s.location.href); }; Services.obs.addObserver(foo, "domwindowopened", false)

in the browser console, and hit cmd-n, I get a log of "about:blank".

Same if I use Services.ww.registerNotification(foo);

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> > What are those cases? From my local testing, it looks like the
> > DOMContentLoaded / load events are fired in the new window before
> > domwindowopened gets sent around.
> 
> That's surprising to me. If I run this:
> 
> foo = (s,t,d) => { s.QueryInterface(Ci.nsIDOMWindow);
> console.log(s.location.href); }; Services.obs.addObserver(foo,
> "domwindowopened", false)
> 
> in the browser console, and hit cmd-n, I get a log of "about:blank".
> 
> Same if I use Services.ww.registerNotification(foo);

So, you're right. It looks like domwindowopened is being sent around before DOMContentLoaded / load, but by the time the test is re-entered having waited for domwindowopened, DOMContentLoaded and load have fired. Odd.

Comment 14

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> > > What are those cases? From my local testing, it looks like the
> > > DOMContentLoaded / load events are fired in the new window before
> > > domwindowopened gets sent around.
> > 
> > That's surprising to me. If I run this:
> > 
> > foo = (s,t,d) => { s.QueryInterface(Ci.nsIDOMWindow);
> > console.log(s.location.href); }; Services.obs.addObserver(foo,
> > "domwindowopened", false)
> > 
> > in the browser console, and hit cmd-n, I get a log of "about:blank".
> > 
> > Same if I use Services.ww.registerNotification(foo);
> 
> So, you're right. It looks like domwindowopened is being sent around before
> DOMContentLoaded / load, but by the time the test is re-entered having
> waited for domwindowopened, DOMContentLoaded and load have fired. Odd.

Promises' then handlers get called in the next turn of the event loop, or at the end of that event loop, or something, right? Maybe that's it? You could deal with it manually and/or add functionality to BrowserTestUtils.jsm that do things directly from the DOMContentLoaded/load event of a new window.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1262946 - Update browser_bug495058.js to account for initial browser focus changes. r?Gijs

Review commit: https://reviewboard.mozilla.org/r/45301/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45301/

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739140 [details]
MozReview Request: Bug 1262946 - Don't focus the initial browser of a new window until it has painted. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45063/diff/2-3/

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739229 [details]
MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45129/diff/2-3/

Comment 18

[Mike Conley (:mconley) (:⚙️)]

This is showing a really excellent win on Windows: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=b6683e141c47&newProject=try&newRevision=653aac9f1f3e&framework=1

It's improving OS X as well, but the mozilla-central base push didn't have any other-e10s builds. I've triggered some.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8739229 [details]
MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

https://reviewboard.mozilla.org/r/45129/#review41787

I'm a little surprised the waitForFocus thing works as-is... and I wonder if correcting it will break anything. Assuming it doesn't, r=me...

::: browser/base/content/test/general/browser.ini:582
(Diff revision 3)
>  [browser_aboutTabCrashed_withoutDump.js]
>  skip-if = !e10s
>  [browser_csp_block_all_mixedcontent.js]
>  tags = mcb
> +[browser_newwindow_focus.js]
> +skip-if = (os == "linux" && !e10s)

Please add a comment with the follow-up you mentioned you were going to file.

::: browser/base/content/test/general/browser_newwindow_focus.js:47
(Diff revision 3)
> +add_task(function* test_focus_browser() {
> +  yield BrowserTestUtils.withNewTab({
> +    url: PAGE,
> +    gBrowser,
> +  }, function*(browser) {
> +    let newWinPromise = BrowserTestUtils.domWindowOpened();

This should probably also use your helper thingy.

::: browser/base/content/test/general/browser_newwindow_focus.js:83
(Diff revision 3)
> +    let newWin = yield newWinPromise;
> +
> +    // Because we're switching focus, we shouldn't steal it once
> +    // content paints.
> +    newWin.gURLBar.focus();
> +    yield new Promise((resolve) => waitForFocus(resolve));

```
yield SimpleTest.promiseFocus(newWin);
```

I'm assuming you want to wait for focus in the new window, not in the original window ? (for which you'd need to pass `newWin` one way or another...)

Comment 20

[:Gijs (he/him)]

Comment on attachment 8739516 [details]
MozReview Request: Bug 1262946 - Update browser_bug495058.js to account for initial browser focus changes. r?Gijs

https://reviewboard.mozilla.org/r/45301/#review41795

::: browser/base/content/test/general/browser_bug495058.js:15
(Diff revision 1)
>      let browser = tab.linkedBrowser;
> -    browser.addEventListener("load", function () {
> -      browser.removeEventListener("load", arguments.callee, true);
> -      detach();
> -    }, true);
>      browser.loadURI(uri);
> -  }
> +    yield BrowserTestUtils.browserLoaded(browser);

Nit:

```
yield BrowserTestUtils.loadURI(tab.linkedBrowser, uri);
```

as `browser` is unused anyway.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/45129/#review41787

> ```
> yield SimpleTest.promiseFocus(newWin);
> ```
> 
> I'm assuming you want to wait for focus in the new window, not in the original window ? (for which you'd need to pass `newWin` one way or another...)

Ah, turns out I don't really need this anyways.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739229 [details]
MozReview Request: Bug 1262946 - Add focus tests for newly opened windows. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45129/diff/3-4/

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739516 [details]
MozReview Request: Bug 1262946 - Update browser_bug495058.js to account for initial browser focus changes. r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45301/diff/1-2/

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67fa8b34b8e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f251fa0a2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5111e423f37d

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

a huge win in tpaint!
https://treeherder.mozilla.org/perf.html#/alerts?id=812

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67fa8b34b8e6
https://hg.mozilla.org/mozilla-central/rev/e5f251fa0a2a
https://hg.mozilla.org/mozilla-central/rev/5111e423f37d

Comment 27

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> Comment on attachment 8739516 [details]
> MozReview Request: Bug 1262946 - Update browser_bug495058.js to account for
> initial browser focus changes. r?Gijs
> 
> https://reviewboard.mozilla.org/r/45301/#review41795
> 
> ::: browser/base/content/test/general/browser_bug495058.js:15
> (Diff revision 1)
> >      let browser = tab.linkedBrowser;
> > -    browser.addEventListener("load", function () {
> > -      browser.removeEventListener("load", arguments.callee, true);
> > -      detach();
> > -    }, true);
> >      browser.loadURI(uri);
> > -  }
> > +    yield BrowserTestUtils.browserLoaded(browser);
> 
> Nit:
> 
> ```
> yield BrowserTestUtils.loadURI(tab.linkedBrowser, uri);
> ```
> 
> as `browser` is unused anyway.

I suspect this is now biting me in bug 1265055 in the trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=badeb83301ad&selectedJob=19553095 , because contrary to what I thought, loadURI doesn't wait for a browser to be loaded at all. :-\