1370035 - Regression: Spacebar pagedown not working after loading session restored tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Asif Youssuff]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170603100243

Steps to reproduce:

1. Start or restart browsing session from stored session
2. switch to a restored but not yet loaded tab by clicking on the tab or switching to it via ctrl-tab
3. Press spacebar key on keyboard


Actual results:

Page does not scroll. 


Expected results:

Page should have scrolled down like it does in release or previous nightlies. 

mozregression localized the bug to: https://bugzilla.mozilla.org/show_bug.cgi?id=1362866

Comment 1

[Asif Youssuff]

18:15.45 INFO: No more inbound revisions, bisection finished.
18:15.45 INFO: Last good revision: 6d4744a6c81c498cc81cad11b4ead64660a3d269
18:15.45 INFO: First bad revision: aa48bb3f494410de514041e8c06ca15c38680181
18:15.45 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6d4744a6c81c498cc81cad11b4ead64660a3d269&tochange=aa48bb3f494410de514041e8c06ca15c38680181

18:16.83 INFO: Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1362866

Comment 2

[Alice0775 White]

I can also reproduce the issue on windows10

Comment 3

[Julien Cristau [:jcristau]]

tracking for 55 as new regression

Comment 4

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

Attachment: Bug 1370035 - Make adjusting focus during a tab switch one of the last things that occurs during any given tick.

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

Comment 5

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

Comment on attachment 8874945 [details]
Bug 1370035 - Make adjusting focus during a tab switch one of the last things that occurs during any given tick.

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

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8874945 [details]
Bug 1370035 - Make adjusting focus during a tab switch one of the last things that occurs during any given tick.

https://reviewboard.mozilla.org/r/146330/#review151264

::: browser/base/content/tabbrowser.xml:1384
(Diff revision 2)
>              focusFlags |= fm.FLAG_SHOWRING;
>          }
>  
> +        // If the browser was just inserted lazily, then it might
> +        // not be focusable until after the next style and layout
> +        // flush. We'll queue up a runnable to focus the browser

Flushing layout implies flushing styles, so you can just say "layout flush"... unless of course a style flush is enough to make the browser focusable (I think it should be).

::: browser/base/content/tabbrowser.xml:1388
(Diff revision 2)
> +        // not be focusable until after the next style and layout
> +        // flush. We'll queue up a runnable to focus the browser
> +        // after the next flush.
> +        let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIDOMWindowUtils);
> +        let r = dwu.getBoundsWithoutFlushing(newBrowser);

let rect = window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIDOMWindowUtils)
                 .getBoundsWithoutFlushing(newBrowser);

::: browser/base/content/tabbrowser.xml:1399
(Diff revision 2)
> +          // it somehow changed between now and the next tick.
> +          document.activeElement.blur();
> +
> +          window.requestAnimationFrame(() => {
> +            Services.tm.dispatchToMainThread(() => {
> +              if (document.activeElement == document.documentElement) {

Should also check that newBrowser is still the selected one here.

::: browser/base/content/tabbrowser.xml:1400
(Diff revision 2)
> +          document.activeElement.blur();
> +
> +          window.requestAnimationFrame(() => {
> +            Services.tm.dispatchToMainThread(() => {
> +              if (document.activeElement == document.documentElement) {
> +                fm.setFocus(newBrowser, focusFlags);

Perhaps setFocus() should just flush style like focus() does?

Comment 7

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

Comment on attachment 8874945 [details]
Bug 1370035 - Make adjusting focus during a tab switch one of the last things that occurs during any given tick.

https://reviewboard.mozilla.org/r/146330/#review151264

> Perhaps setFocus() should just flush style like focus() does?

I'm very hestitant to add yet another place where we can flush - even if it's just styles. Are you fully opposed to just waiting for the next tick?

Comment 8

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

ni? for dao for comment 7.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #7)
> Comment on attachment 8874945 [details]
> Bug 1370035 - Make sure lazily inserted browsers are focused after tab
> switch.
> 
> https://reviewboard.mozilla.org/r/146330/#review151264
> 
> > Perhaps setFocus() should just flush style like focus() does?
> 
> I'm very hestitant to add yet another place where we can flush - even if
> it's just styles. Are you fully opposed to just waiting for the next tick?

It's not a great pattern as it can race with other focus calls. It's possible to manage this by blurring and later checking that focus hasn't changed (and that you still want to focus the thing you originally wanted to focus, see my other review comment), but it seems like a footgun. Is there a good reason why focus would flush styles but setFocus wouldn't? FWIW, the only reason we're using setFocus here is to use FLAG_NOSCROLL and FLAG_SHOWRING, not because we're interested in other behavioral differences. (Apparently FLAG_SHOWRING isn't used with e10s. Not sure what difference FLAG_NOSCROLL would make here. Maybe we can just get rid of this.)

Comment 10

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

(In reply to Dão Gottwald [::dao] from comment #9)
> It's not a great pattern as it can race with other focus calls. It's
> possible to manage this by blurring and later checking that focus hasn't
> changed (and that you still want to focus the thing you originally wanted to
> focus, see my other review comment), but it seems like a footgun. Is there a
> good reason why focus would flush styles but setFocus wouldn't?

There seems to be another factor here that I can't figure out... even if I force a _full layout_ flush either in nsFocusManager::CheckIfFocusable or manually via newBrowser.clientTop in _adjustFocusAfterTabSwitch, we fail to find the primary frame for the newBrowser here:

http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/dom/base/nsFocusManager.cpp#1590-1594

Digging into it more, it seems that this is because SessionStore hasn't removed the "pending" attribute from the browser node, so it's still display: none, so it cannot be focused.

This, and bug 1370518 are both known fallout from moving the focus switch to _before_ the call to updateCurrentBrowser (which fires the TabSelect event). There might be some unknown fallout here. Not too jazzed about that.

I'm going to see if there's a way we can defer the adjustment of focus until after the updateCurrentBrowser call...

Comment 11

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

Comment on attachment 8874945 [details]
Bug 1370035 - Make adjusting focus during a tab switch one of the last things that occurs during any given tick.

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

Comment 12

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

Our focus stuff is such a mess. :( This is the best thing I could come up with. Eager for your feedback, dao.

Comment 13

[Dão Gottwald [:dao]]

Can we remove the pending attribute earlier? Alternatively, can we remove http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/browser/base/content/tabbrowser.css#92-100 now that tabs are "lazy" before becoming "pending"?

Comment 14

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

Attachment: Bug 1370035 - Remove pending tab optimization now that tabs are injected lazily.

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

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8877311 [details]
Bug 1370035 - Remove pending tab optimization now that tabs are injected lazily.

https://reviewboard.mozilla.org/r/148646/#review153416

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4180fb6074b1
Remove pending tab optimization now that tabs are injected lazily. r=dao

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/4180fb6074b1

Comment 18

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

Comment on attachment 8877311 [details]
Bug 1370035 - Remove pending tab optimization now that tabs are injected lazily.

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1362866

[User impact if declined]:

Users that restore tabs from a previous session on demand will find that when they first select an unrestored tab, that the browser doesn't get focus.

One manifestation of this bug is that if you select an unrestored tab, and hit Spacebar, the page won't page down until you focus the browser manually (or switch tabs away and back).

[Is this code covered by automated tests?]:

Tab switching and browser restoration is. This particular bug doesn't have a regression test, but it should. I've filed bug 1373665 to do that.

[Has the fix been verified in Nightly?]:

Yes, I myself was able to reproduce the issue locally and have verified the fix.

[Needs manual test from QE? If yes, steps to reproduce]: 

It couldn't hurt. STR:

1) Open one or two new tabs and browse to some websites that are long enough to require you to scroll.
2) Switch back to the first tab.
3) Restart Firefox
4) Restore your previous session if you're not configured to do that automatically.
5) Switch to one of the tabs you'd opened previously by clicking on the tab.
6) Once the page is loaded, press the Spacebar.

ER: The page, once restored, should page down once you press the Spacebar

AR: The page does not page down.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're removing the impediment to the focus, which is an old optimization that is no longer required.

[String changes made/needed]:

None.

Comment 19

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

Regression in 55, let's uplift this fix and test the fix in beta.

Comment 20

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

Asif, is this the first bug you found that got fixed? I think maybe it is. Congrats! Thanks so much for your testing!

Comment 21

[Asif Youssuff]

Sorry to disappoint Liz, but I think the first bug that I can count as having been fixed is bug 1165611. 

But as always, I continue to be impressed by the Firefox team's commitment to quality and community involvement. Thanks so much for continuing to produce this great browser!

Looking forward to Firefox 57!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ee30fad7cb7

Comment 23

[Ciprian Georgiu, Desktop QA]

I managed to reproduce this issue using a Nightly build from 2017-06-03, by following the STR from comment 18.

This is verified fixed on 55 beta 3 (20170619141703) and latest Nightly 56.0a1 (2017-06-20) across platforms: Windows 10 x64, macOS 10.12.5 and Ubuntu 16.04 x64 LTS.