Closed Bug 1370035 Opened 2 years ago Closed 2 years ago

Regression: Spacebar pagedown not working after loading session restored tab

Categories

(Firefox :: Tabbed Browser, defect)

55 Branch
Unspecified
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified
firefox56 --- verified

People

(Reporter: yoasif, Assigned: mconley)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
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
Component: Untriaged → Tabbed Browser
Keywords: regression
I can also reproduce the issue on windows10
Has Regression Range: yes → ---
Has STR: yes → ---
OS: Unspecified → All
Blocks: 1362866
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(mconley)
Has Regression Range: --- → yes
Has STR: --- → yes
Assignee: nobody → mconley
tracking for 55 as new regression
Flags: needinfo?(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

::: 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?
Attachment #8874945 - Flags: review?(dao+bmo) → review-
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?
ni? for dao for comment 7.
Flags: needinfo?(dao+bmo)
(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.)
Flags: needinfo?(dao+bmo)
(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...
See Also: → 1371884
Our focus stuff is such a mess. :( This is the best thing I could come up with. Eager for your feedback, 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"?
Flags: needinfo?(mconley)
Attachment #8874945 - Attachment is obsolete: true
Attachment #8874945 - Flags: review?(dao+bmo)
Flags: needinfo?(mconley)
Comment on attachment 8877311 [details]
Bug 1370035 - Remove pending tab optimization now that tabs are injected lazily.

https://reviewboard.mozilla.org/r/148646/#review153416
Attachment #8877311 - Flags: review?(dao+bmo) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4180fb6074b1
Remove pending tab optimization now that tabs are injected lazily. r=dao
https://hg.mozilla.org/mozilla-central/rev/4180fb6074b1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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.
Attachment #8877311 - Flags: approval-mozilla-beta?
Regression in 55, let's uplift this fix and test the fix in beta.
Flags: qe-verify+
Attachment #8877311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Asif, is this the first bug you found that got fixed? I think maybe it is. Congrats! Thanks so much for your testing!
Flags: needinfo?(yoasif)
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!
Flags: needinfo?(yoasif)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1418038
You need to log in before you can comment on or make changes to this bug.