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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Ryan Feeley [:rfeeley]]

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

Comment 1

[:Gijs (out for now; he/him)]

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.

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[:Gijs (out for now; he/him)]

(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.

Comment 4

[:Gijs (out for now; he/him)]

Attachment: Bug 1391280 - store last sidebar command irrespective of whether sidebar was open,

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

Comment 5

[Brian Grinstead [:bgrins]]

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

Comment 6

[:Gijs (out for now; he/him)]

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

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

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[:Gijs (out for now; he/him)]

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.

Comment 9

[:Gijs (out for now; he/him)]

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 10

[:Gijs (out for now; he/him)]

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

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f5d47ac123
store last sidebar command irrespective of whether sidebar was open, r=bgrins

Comment 12

[Pulsebot]

Backout by [e-mail]:
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

[April King [:April]]

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.

Comment 14

[Brian Grinstead [:bgrins]]

(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

[April King [:April]]

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

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Pulsebot from comment #12)
> Backout by [e-mail]:
> 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?

Comment 17

[:Gijs (out for now; he/him)]

In fact, it probably makes more sense to just update the patch and re-request review, so clearing ni for now. :-)

Comment 18

[Brian Grinstead [:bgrins]]

(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.

Comment 19

[:Gijs (out for now; he/him)]

(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 20

[:Gijs (out for now; he/him)]

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

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

Comment 21

[:Gijs (out for now; he/him)]

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

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

Comment 22

[:Gijs (out for now; he/him)]

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.

Comment 23

[Brian Grinstead [:bgrins]]

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`.

Comment 24

[Shane Caraveo (:mixedpuppy)]

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

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

Comment 25

[:Gijs (out for now; he/him)]

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 26

[:Gijs (out for now; he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171648/diff/5-6/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cd2b311c3fb
store last sidebar command irrespective of whether sidebar was open, r=bgrins,mixedpuppy

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/0cd2b311c3fb

Comment 29

[Emil Ghitta, QA [:emilghitta]]

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.