The default bug view has changed. See this FAQ.

Session Restore moves windows around unnecessarily

NEW
Unassigned

Status

()

Firefox
Session Restore
6 years ago
2 months ago

People

(Reporter: int3, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 9
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
STR:

1. Set Preferences -> When Firefox starts -> Show my home page, with about:home as the home page
2. Open two firefox windows on different parts of the screen. Let us label the the two positions as A and B.
3. Quit firefox with the last focused window being located at position A.
4. Reopen firefox, and click on the 'Restore Previous Session' button that appears.

Actual Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, that window is moved to B, and another window is opened at A. This new window grabs the focus.

Expected Results:
When firefox is reopened, the new window is positioned at A. When the 'Restore Previous Session' button is clicked, another window is opened at B. The window at A retains the focus throughout.

The current behavior produces a noticeable 'flicker' as the window at A is moved away and a new one created in its place. Also, since the user cannot effectively interact with the browser until the focus stops changing, and since creating new windows is a relatively slow step, this makes the restore process feel slower.

This behavior is also observed if one selects the preference 'Show my windows and tabs from last time'.

Comment 1

6 years ago
WFM on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110705 Firefox/7.0a1.

When I've restored the session, the windows retained their original position and focus.

Also, could you see if the issue occurs if using Firefox in safe mode:
http://support.mozilla.com/kb/Safe+Mode

Could you see if the issue occurs if using an empty testing profile? (Don't install any addons into it)
http://support.mozilla.com/kb/Basic+Troubleshooting#w_8-make-a-new-profile
(Reporter)

Comment 2

6 years ago
Created attachment 545753 [details] [diff] [review]
Patch v1
(Reporter)

Comment 3

6 years ago
Created attachment 545754 [details] [diff] [review]
Patch v1
Attachment #545753 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #545754 - Flags: review?(paul)

Comment 4

6 years ago
Considering comment!, setting resolution to Resolved WFM.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 5

6 years ago
Vlad: Sorry for not replying with more details -- yes, the issue occurs with a clean profile. The issue might not occur 100% of the time though -- I think there's the possibility that the windows indeed get restored without unnecessary movement; however, the code definitely doesn't guarantee that this happens. My patch should fix that.
Assignee: nobody → jezreel
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---

Comment 6

6 years ago
Ok. Waiting for the patch.
I have tried on the latest nightly and is still WFM.
(Reporter)

Comment 7

6 years ago
The patch has already been submitted and is awaiting review. See above.
Comment on attachment 545754 [details] [diff] [review]
Patch v1

Review of attachment 545754 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple nits.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1474,5 @@
>      // the lastWindow.
>      let lastWindow = this._getMostRecentBrowserWindow();
>      let canUseLastWindow = lastWindow &&
>                             !lastWindow.__SS_lastSessionWindowID;
> +    let lastSessionFocusedWindow = null;

My gut wants to say that we don't actually need lastSessionFocusedWindow, but we do.

@@ +1518,5 @@
>          //        in _preWindowToRestoreInto will prevent most (all?) Panorama
>          //        weirdness but we will still merge other extData.
>          //        Bug 588217 should make this go away by merging the group data.
>          this.restoreWindow(windowToUse, { windows: [winState] }, canOverwriteTabs, true);
> +        if (i == 0) lastSessionFocusedWindow = windowToUse;

Nit: 2 line that if

@@ +1528,4 @@
>        }
>        else {
> +        let win = this._openWindowWithState({ windows: [winState] });
> +        if (i == 0) lastSessionFocusedWindow = win;

Nit: 2 line that if

@@ +2485,5 @@
>      var winData;
> +    if (!root.selectedWindow) {
> +      root.selectedWindow = 0;
> +    } else {
> +      root.windows.unshift(root.windows.splice(root.selectedWindow - 1, 1)[0]);

Add a comment here that this makes sure the selected window gets restored first
Attachment #545754 - Flags: review?(paul) → review+
(Reporter)

Comment 9

6 years ago
Created attachment 558137 [details] [diff] [review]
Patch v2

Patch with comments addressed.
Attachment #545754 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76
http://hg.mozilla.org/integration/mozilla-inbound/rev/5312c652c469
Target Milestone: --- → Firefox 9
http://hg.mozilla.org/mozilla-central/rev/5312c652c469
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 14

6 years ago
Hi guys.
I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Every thing works as expected except the fact that after step 4 from the description, I don't have the 'Restore Previous Session' button in the page, only in the menu. If have try this on a clean profile.

Is this the intended behavior?

Thanks
(In reply to Vlad [QA] from comment #14)
> Hi guys.
> I am trying to verify this on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:9.0a1) Gecko/20110907 Firefox/9.0a1
> 
> Every thing works as expected except the fact that after step 4 from the
> description, I don't have the 'Restore Previous Session' button in the page,
> only in the menu. If have try this on a clean profile.
> 
> Is this the intended behavior?
> 
> Thanks

This bug is more about the behaviour of windows when restored. Use whatever method you can to restore the session. It's not clear to me whether step 4 refers to the Restore Session link in the home page or the Restore Previous Session menuitem in the History menu.
(In reply to Vlad [QA] from comment #14)
> Is this the intended behavior?

No, but it's not directly related to this bug. It may be bug 682944.

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #15)
> It's not clear to me whether step 4
> refers to the Restore Session link in the home page or the Restore Previous
> Session menuitem in the History menu.

For the record, it doesn't actually matter.
Flags: in-testsuite-

Comment 17

6 years ago
Considering comment14, comment15 and comment16, setting resolution to Verified Fixed.
Status: RESOLVED → VERIFIED
Depends on: 694378

Updated

5 years ago
Depends on: 712763
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/66295fcfee13
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Back out is now on m-c:

https://hg.mozilla.org/mozilla-central/rev/66295fcfee13
jez, anyone,
do you still see this?  or Bug 1235231?
personally, I haven't. on Windows anyway
Assignee: jezreel → nobody
Blocks: 1330635
Status: REOPENED → NEW
You need to log in before you can comment on or make changes to this bug.