Show sidebars button should show previously opened sidebar, even after closing the sidebar and a restart

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
10 months ago
8 months ago

People

(Reporter: rfeeley, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
This bug is based on feedback received during Test Pilot Notes where users have to put in extra effort to return to their last used sidebar (Notes).

STEPS TO REPRODUCE
1. Open a sidebar using the sidebar toolbar button
2. Swith to a sidebar that isn't bookmarks
3. Restart Firefox
4. Click on sidebar toolbar button

EXPECTED RESULTS
- Last opened sidebar is opened

ACTUAL RESULTS
- Bookmarks is opened
(Assignee)

Comment 1

10 months ago
Is this on release? I'm confused about the steps:

(In reply to Ryan Feeley [:rfeeley] from comment #0)
> This bug is based on feedback received during Test Pilot Notes where users
> have to put in extra effort to return to their last used sidebar (Notes).
> 
> STEPS TO REPRODUCE
> 1. Open a sidebar using the sidebar toolbar button
> 2. Swith to a sidebar that isn't bookmarks
> 3. Restart Firefox

On nightly, after following those steps, the sidebar is still open for me.
Flags: needinfo?(rfeeley)
It may be missing a step between 2 and 3 for 'close the sidebar' - if I do that I see the Bookmarks reopened. And it makes sense that this happens due to the hide function clearing out the sidebarcommand attribute on the box.
(Assignee)

Comment 3

10 months ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> It may be missing a step between 2 and 3 for 'close the sidebar' - if I do
> that I see the Bookmarks reopened. And it makes sense that this happens due
> to the hide function clearing out the sidebarcommand attribute on the box.

Thanks! Yes, this helps.
Blocks: 1353421, 1387512
Flags: needinfo?(rfeeley)
Summary: Show sidebars button should show previously opened sidebar, even after restart → Show sidebars button should show previously opened sidebar, even after closing the sidebar and a restart
(Assignee)

Updated

10 months ago
Flags: qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [reserve-photon-structure]
Version: 55 Branch → 57 Branch
(Assignee)

Updated

10 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Comment hidden (mozreview-request)

Comment 5

10 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review176820

Thank you for fixing this! Can you please update https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_sidebar_toggle.js or add a new test in browser/base/content/test/sidebar to cover this case?

::: browser/base/content/browser-sidebar.js:269
(Diff revision 1)
>    /**
>     * The ID of the current sidebar (ie, the ID of the broadcaster being used).
>     * This can be set even if the sidebar is hidden.
>     */
>    get currentID() {
> -    return this._box.getAttribute("sidebarcommand");
> +    return this._box.hidden ? "" : this._box.getAttribute("sidebarcommand");

Nit: there's an isOpen getter

::: browser/base/content/browser-sidebar.js:447
(Diff revision 1)
>      this.browser.setAttribute("src", "about:blank");
>      this.browser.docShell.createAboutBlankContentViewer(null);
>  
>      sidebarBroadcaster.removeAttribute("checked");
> -    this._box.setAttribute("sidebarcommand", "");
> +    this._box.setAttribute("sidebaropen", "false");
>      this._box.removeAttribute("checked");

Too bad we can't persist removed attributes, otherwise we could use [checked] instead of needing a new one
Attachment #8900258 - Flags: review?(bgrinstead)

Updated

10 months ago
Priority: P3 → P1
Comment hidden (mozreview-request)

Comment 7

10 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review177672

Thank you, this is a big improvement

::: browser/base/content/browser-sidebar.js:67
(Diff revision 2)
>        this.toggleSwitcherPanel();
>      });
>    },
>  
>    uninit() {
> -    let enumerator = Services.wm.getEnumerator(null);
> +    let enumerator = Services.wm.getEnumerator("navigator:browser");

What's the purpose of this change? I assume this has to do with window.opener somehow.

::: browser/base/content/browser-sidebar.js:272
(Diff revision 2)
>      return !this._box.hidden;
>    },
>  
>    /**
>     * The ID of the current sidebar (ie, the ID of the broadcaster being used).
>     * This can be set even if the sidebar is hidden.

This comment is out of date now, currentID returns an empty string if the sidebar is hidden

::: browser/base/content/browser-sidebar.js:372
(Diff revision 2)
>  
>        this.hideSwitcherPanel();
>  
>        this._box.setAttribute("checked", "true");
>        this._box.setAttribute("sidebarcommand", sidebarBroadcaster.id);
> -      this.lastOpenedId = sidebarBroadcaster.id;
> +      this._box.setAttribute("sidebaropen", "true");

I wish we didn't have to track three different things for 'is this hidden?' (`this._box.hidden`, `:not([checked])`, and `[sidebaropen=false]`).

I know there's a separate reason for each, but with a similar workaround for xul store removal on `[checked]` as in Bug 1391549, perhaps we could skip `[sidebaropen]` altogether and rely only on `[checked]`? I don't feel too strongly about it, so r=me either way.
Attachment #8900258 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 8

10 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review177838

::: browser/base/content/browser-sidebar.js:67
(Diff revision 2)
>        this.toggleSwitcherPanel();
>      });
>    },
>  
>    uninit() {
> -    let enumerator = Services.wm.getEnumerator(null);
> +    let enumerator = Services.wm.getEnumerator("navigator:browser");

I've tried to add a comment here. The previous code wouldn't have persisted things in the following situation:

1) open firefox
2) open browser console, bookmarks library window, about dialog, password manager, or anything else that opens as a window but isn't modal
3) close browser window

Now the browser window would close but we would "count" the other non-browser windows when determining whether this was the last window and we should persist sidebar state. Only enumerating browser windows fixes this.
(Assignee)

Comment 9

10 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review177842

::: browser/base/content/browser-sidebar.js:372
(Diff revision 2)
>  
>        this.hideSwitcherPanel();
>  
>        this._box.setAttribute("checked", "true");
>        this._box.setAttribute("sidebarcommand", sidebarBroadcaster.id);
> -      this.lastOpenedId = sidebarBroadcaster.id;
> +      this._box.setAttribute("sidebaropen", "true");

Good point, unified with [checked].
Comment hidden (mozreview-request)

Comment 11

10 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f5d47ac123
store last sidebar command irrespective of whether sidebar was open, r=bgrins

Comment 12

10 months ago
Backout by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9994a27fda2
Back out b5f5d47ac123 for breaking when session store tries to restore sidebars (latent errors in webextension test browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_lazy.js ) on a CLOSED TREE

Comment 13

10 months ago
Right now I'm using Tab Center Redux, and for some reason Firefox always restarts with the Tab sidebar closed. I'm not sure if this is the same bug or not, so I thought I'd post here first so as to avoid the duplicate.
(In reply to April King [:April] from comment #13)
> Right now I'm using Tab Center Redux, and for some reason Firefox always
> restarts with the Tab sidebar closed. I'm not sure if this is the same bug
> or not, so I thought I'd post here first so as to avoid the duplicate.

This sounds like Bug 1385630, which was resolved a few days ago. But if you are still seeing this on a Nightly from the last couple days could you leave a comment there?

Comment 15

10 months ago
Ahh, perfect. It is indeed still happening, so I'll comment over there. Thanks for pointing me in that direction!

Updated

10 months ago
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
(Assignee)

Comment 16

10 months ago
(In reply to Pulsebot from comment #12)
> Backout by gijskruitbosch@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b9994a27fda2
> Back out b5f5d47ac123 for breaking when session store tries to restore
> sidebars (latent errors in webextension test
> browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_tabs_lazy.js ) on a CLOSED TREE

So the reason this didn't work out when landing is that sessionstore independently tries to reopen the last-opened sidebar for each window.

This is unfortunate. I could have wallpapered this with a followup fix of some sort, but I wanted to think about this a bit more.

We end up with a conflicting situation where both XUL store persistence are saving the 'last sidebar', and sessionstore does the same but on a per-window basis.

It looks as if the status quo is just that session store will open sidebars that might otherwise not be reopened, that is, if you follow these steps:

1. open firefox, clean profile
2. turn on session restore
3. open the bookmarks sidebar in that window
4. open a new window
5. close the bookmarks sidebar in that new window (but keep it open in the old one)
6. exit/quit firefox (closing both windows in 1 go using file > exit / app menu > quit)
7. restart

now we won't have persisted an open sidebar, and session store will open one in the background window anyway because it knew about that one.

However, if you follow these steps:


1. open firefox, clean profile
2. turn on session restore
3. open a new window
4. open the bookmarks sidebar in that window
5. close the bookmarks sidebar in that new window (but keep it open in the old one)
6. exit/quit firefox (closing both windows in 1 go using file > exit / app menu > quit)
7. restart

now both windows will open a sidebar, because xul storage "overrides" sessionstore's knowledge that no sidebar was open in the first window.

Ultimately, I don't think there is a great solution here - we can't reliably turn XUL storage of these attributes off depending on whether session restore is in use (and in some cases session restore will be used even if it's generally off, like restarting to update) and removing the XUL storage code altogether will break things for users who don't use session restore (which is most users). No longer storing sidebar state in session restore would break things for users in some cases... and that code goes back to at least hg@1, so it's basically more than 10 years old at this point.

So, I think what I'll do is attempt to fix session restore to work in the simplest way possible here (by also checking the 'checked' state and not just the sidebarcommand state) and then any other work we might want to do here (if anyone can come up with a solid plan - I'm struggling...) can be a followup bug.

Brian, does that make sense to you?
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 17

10 months ago
In fact, it probably makes more sense to just update the patch and re-request review, so clearing ni for now. :-)
Flags: needinfo?(bgrinstead)
(https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3034).
(In reply to :Gijs (queue backed up, slow) from comment #16)
> No longer storing sidebar state in session restore
> would break things for users in some cases... and that code goes back to at
> least hg@1, so it's basically more than 10 years old at this point.

So if we removed the session store code, the breakage would be situations where you have disabled the sidebar on one window but enabled it in another? IIUC then in this case upon restarting w/ session restore either all the windows will have sidebars or none, depending on the last change.

> So, I think what I'll do is attempt to fix session restore to work in the
> simplest way possible here (by also checking the 'checked' state and not
> just the sidebarcommand state) and then any other work we might want to do
> here (if anyone can come up with a solid plan - I'm struggling...) can be a
> followup bug.
> 
> Brian, does that make sense to you?

I think that makes sense. We're changing the semantics of [sidebarcommand] here so we should also update the session store usage of it to [checked] as you mention.
(Assignee)

Comment 19

10 months ago
(In reply to Brian Grinstead [:bgrins] from comment #18)
> (https://dxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/SessionStore.jsm#3034).
> (In reply to :Gijs (queue backed up, slow) from comment #16)
> > No longer storing sidebar state in session restore
> > would break things for users in some cases... and that code goes back to at
> > least hg@1, so it's basically more than 10 years old at this point.
> 
> So if we removed the session store code, the breakage would be situations
> where you have disabled the sidebar on one window but enabled it in another?
> IIUC then in this case upon restarting w/ session restore either all the
> windows will have sidebars or none, depending on the last change.

Yeah, exactly. Which, I dunno, is kind of more sane than the random behaviour we have now, but arguably not the best either.

> > So, I think what I'll do is attempt to fix session restore to work in the
> > simplest way possible here (by also checking the 'checked' state and not
> > just the sidebarcommand state) and then any other work we might want to do
> > here (if anyone can come up with a solid plan - I'm struggling...) can be a
> > followup bug.
> > 
> > Brian, does that make sense to you?
> 
> I think that makes sense. We're changing the semantics of [sidebarcommand]
> here so we should also update the session store usage of it to [checked] as
> you mention.

Yeah... I think in the ideal case, we would somehow ensure the persist-based store always gets overridden by session store (even in the case where persist thinks it ought to open a sidebar and session store knows it shouldn't), but if we want to do that we still need to update session store to take [checked] into account anyway, and so we can reasonably follow-up the "make session store and xul persistence of the sidebar state not clash" bit, which seems racy and headachy for other reasons... though it's good to know this clash exists because it might tie into bug 1385630.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

9 months ago
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

Re-requesting review because I changed the patch significantly as a result of bug 1385630.
Attachment #8900258 - Flags: review+ → review?(bgrinstead)

Comment 23

9 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review184186

The changes look fine to me

::: browser/base/content/browser-sidebar.js:251
(Diff revision 5)
>      } else {
> +      this._box.removeAttribute("checked");
>        // Remove the |sidebarcommand| attribute, because the element it
>        // refers to no longer exists, so we should assume this sidebar
>        // panel has been uninstalled. (249883)
>        this._box.removeAttribute("sidebarcommand");

Unrelated to this change, but I notice that we do `document.persist("sidebar-box", "sidebarcommand");` in uninit which means that removing the attribute here won't persist (the line being deleted in `hide` did `this._box.setAttribute("sidebarcommand", "")` instead).

I'm not sure that this really matters in practice, as next time we are in this function we'll short circuit in `!wasOpen` since we are removing (and persisting the removal of) `checked`.
Attachment #8900258 - Flags: review?(bgrinstead) → review+

Comment 24

9 months ago
mozreview-review
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review184528
Attachment #8900258 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 25

9 months ago
mozreview-review-reply
Comment on attachment 8900258 [details]
Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

https://reviewboard.mozilla.org/r/171648/#review184186

> Unrelated to this change, but I notice that we do `document.persist("sidebar-box", "sidebarcommand");` in uninit which means that removing the attribute here won't persist (the line being deleted in `hide` did `this._box.setAttribute("sidebarcommand", "")` instead).
> 
> I'm not sure that this really matters in practice, as next time we are in this function we'll short circuit in `!wasOpen` since we are removing (and persisting the removal of) `checked`.

Updated this to also use setAttribute, and added a comment as to why.
Comment hidden (mozreview-request)

Comment 27

9 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0cd2b311c3fb
store last sidebar command irrespective of whether sidebar was open, r=bgrins,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/0cd2b311c3fb
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

9 months ago
Depends on: 1401232
(Assignee)

Updated

8 months ago
Depends on: 1406824
I have reproduced the issue mentioned in comment 0 using an affected Firefox 57.0a1 build (BuildId:20170817100132).

I have verified that the issue is not reproducible using Firefox 57.0b7 (Build Id:20171009192146) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.