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

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.

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.

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

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

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

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?

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

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

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

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

(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 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 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 on attachment 8900258 [details] Bug 1391280 - store last sidebar command irrespective of whether sidebar was open, https://reviewboard.mozilla.org/r/171648/#review184528

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.

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

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.