Closed Bug 1360282 Opened 7 years ago Closed 7 years ago

Remove the panel that opens when clicking on the sidebar toolbar button

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [photon-structure])

Attachments

(5 files, 1 obsolete file)

Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
After we have a switcher available within the sidebar, the button on the toolbar should turn into a more simple toggle button.

See https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252081
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Aaron, how should this interact with the webextension integration with sidebars?  As far as I can tell from the spec, we want the sidebar toolbar button to go away from having the popup panel that lists sidebars and instead become a toggle.

For web extensions see Bug 1208596, specifically https://bugzilla.mozilla.org/attachment.cgi?id=8818986 which shows the sidebarAction API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction.  There's also more info at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Sidebars.

Should we move the additional menu items that the addons created into the sidebar switcher popup?
Flags: needinfo?(abenson)
Yes, the toolbar button menu is basically being replaced by the switcher in the top of the sidebar. I've added to the spec more details about this: https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/231191185
Flags: needinfo?(abenson)
Blocks: 1364238
Depends on: 1365637
The web extension part will be handled in bug 1365637 so this bug can be focused on just removing the panel and switching the toolbar button to a toggle
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

https://reviewboard.mozilla.org/r/142960/#review146680

::: browser/base/content/browser-sidebar.js
(Diff revision 2)
> -                             sourceUI._title.getAttribute("value"));
>      this._box.setAttribute("width", sourceUI._box.boxObject.width);
> +    this.show(commandID);
>  
> -    this._box.setAttribute("sidebarcommand", commandID);
> -    // Note: we're setting 'src' on this._box, which is a <vbox>, not on

This comment indicates that we set and persist "src" on the box in order to delay loading the browser, but just a few lines later we load the browser (line 182). Because of that I've removed that special handling from the file and rely instead just on setting "src" on the browser inside show()
Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

I'd like some feedback on this refactor patch which will make it easier to implement this change.

Basically, to switch the toolbarbutton to a toggle we'll need to know which sidebar was last opened. This is currently tracked in one of two ways:
1) this.lastOpenedId: this is set on show(), but show() isn't called in either adoptFromWindow (if this is a new window created with an opened) or startDelayedLoad (if the sidebar is opened at startup).  Which means if the sidebar was opened initially then you pressed the toggle button to close and pressed it again, we wouldn't know which sidebar to open.
2) this._box.getAttribute("sidebarcommand"): this gets cleared out in hide(). We could not clear it out, but that would take extra work since it's persisted and used to decide which tool to reopen for a new window

This patch routes all opening paths through the show() function (on startup, as a new window with an opener, or due to a command).  This will let us access lastOpenedId consistently so that the last sidebar shown will be opened after pressing the toolbar button.

Just asking for feedback since show() does more things than adoptFromWindow /  startDelayedLoad did.  Most notably:
1) Fire Sidebar:VisibilityChange
2) Fire SidebarFocused
3) BrowserUITelemetry.countSidebarEvent(commandID, "show");

I'm fairly sure we will want to opt out of the telemetry part when it's called from adoptFromWindow / startDelayedLoad, but I'm not sure that the extra events hurt anything (at least, mochitests stay green)
Attachment #8871495 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

https://reviewboard.mozilla.org/r/142960/#review146680

> This comment indicates that we set and persist "src" on the box in order to delay loading the browser, but just a few lines later we load the browser (line 182). Because of that I've removed that special handling from the file and rely instead just on setting "src" on the browser inside show()

Wuh. I half-noticed this in bug 1208596 comment 41, but not fully, apparently. :-\
Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8871495 [details]
> Bug 1360282 - Always call `show` to open the sidebar, even when it's called
> from startDelayedLoad
> 
> I'd like some feedback on this refactor patch which will make it easier to
> implement this change.
> 
> Basically, to switch the toolbarbutton to a toggle we'll need to know which
> sidebar was last opened. This is currently tracked in one of two ways:
> 1) this.lastOpenedId: this is set on show(), but show() isn't called in
> either adoptFromWindow (if this is a new window created with an opened) or
> startDelayedLoad (if the sidebar is opened at startup).  Which means if the
> sidebar was opened initially then you pressed the toggle button to close and
> pressed it again, we wouldn't know which sidebar to open.
> 2) this._box.getAttribute("sidebarcommand"): this gets cleared out in
> hide(). We could not clear it out, but that would take extra work since it's
> persisted and used to decide which tool to reopen for a new window
> 
> This patch routes all opening paths through the show() function (on startup,
> as a new window with an opener, or due to a command).  This will let us
> access lastOpenedId consistently so that the last sidebar shown will be
> opened after pressing the toolbar button.
> 
> Just asking for feedback since show() does more things than adoptFromWindow
> /  startDelayedLoad did.  Most notably:
> 1) Fire Sidebar:VisibilityChange

This is dead code - it was added in https://hg.mozilla.org/mozilla-central/rev/0ca14dd6fa2a but then we removed reading list. We should just rm it.

> 2) Fire SidebarFocused

This isn't dead. The worst that I can think of this doing is stealing focus from the location bar if the bookmarks/history toolbar is opened when the window opens (or focus moving excessively). I don't think adding pre-emptive specialcasing here is a good idea unless we really need to, though. I'd just do this and then see what breaks.

> 3) BrowserUITelemetry.countSidebarEvent(commandID, "show");
> 
> I'm fairly sure we will want to opt out of the telemetry part when it's
> called from adoptFromWindow / startDelayedLoad, but I'm not sure that the
> extra events hurt anything (at least, mochitests stay green)

Yes, I think the telemetry we should probably opt out of.

I haven't checked other stuff in detail, but this approach seems eminently sensible based on your explanation (and, at a glance, looks like code removal / simplification, so yay for that! :-)
Attachment #8871495 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
The telemetry is not submitted only from the public `show` call, while calls on startup / adopting reference the internal `_show`  which does the same but without the telemetry.  The next 3 patches are just some cleanups I noticed when working on the first.

Holding off on requesting review for the last until I get tests fixed.
Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

https://reviewboard.mozilla.org/r/142960/#review150678

Tentative r+ though this should probably not land until next week. I *am* a bit concerned about the sidebar persistence across process restarts at the moment, but I'm assuming I'm just missing something?

::: commit-message-23a34:3
(Diff revision 5)
> +Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;r=Gijs
> +
> +By running through the same path anytime the side is shown it will be easier to

Nit: the sidebar is shown

::: commit-message-23a34:5
(Diff revision 5)
> +Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;r=Gijs
> +
> +By running through the same path anytime the side is shown it will be easier to
> +implement the sidebar-button as a toggle, since we can rely on consistent state
> +to determined which sidebar was opened last

Nit: determine

Nit: add '.' at the end.

::: browser/base/content/browser-sidebar.js
(Diff revision 5)
> -    this._title.setAttribute("value",
> -                             sourceUI._title.getAttribute("value"));

I love how this object has public getters/setters for this stuff but we were using `_underscore` accessors anyway. Good riddance.

::: browser/base/content/browser-sidebar.js:287
(Diff revision 5)
>     *
> +   * This wraps the internal method, including a ping to telemetry.
> +   *
>     * @param {string} commandID ID of the xul:broadcaster element to use.
>     */
>    show(commandID) {

Nit:
```js
async show(commandID) {
  await this._show(commandID);
  BrowserUITelemetry.countSidebarEvent(commandID, "show");
}
```

Also, not your bug, but should this fire for private windows? Please file a followup if you think the answer is 'no'.

::: browser/base/content/browser-sidebar.js:336
(Diff revision 5)
> -      }
> +      // When loading a web page in the sidebar there is no title set on the
> +      // broadcaster, as it is instead set by openWebPanel. Don't clear out
> +      // the title in this case.

But are there other cases where there is no title? Should we set it to a default ("Sidebar") or something?

::: browser/base/content/browser-sidebar.js
(Diff revision 5)
> -      // We set this attribute here in addition to setting it on the <browser>
> -      // element itself, because the code in SidebarUI.uninit() persists this
> -      // attribute, not the "src" of the <browser id="sidebar">. The reason it

I'm sure I'm being dumb, but where are we persisting the sidebar state between processes now, particularly for bookmarks loaded in the sidebar?
Attachment #8871495 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

https://reviewboard.mozilla.org/r/143254/#review150686
Attachment #8871822 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

https://reviewboard.mozilla.org/r/143252/#review150688
Attachment #8871821 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

https://reviewboard.mozilla.org/r/143292/#review150690
Attachment #8871823 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #22)
> ::: browser/base/content/browser-sidebar.js
> (Diff revision 5)
> > -      // We set this attribute here in addition to setting it on the <browser>
> > -      // element itself, because the code in SidebarUI.uninit() persists this
> > -      // attribute, not the "src" of the <browser id="sidebar">. The reason it
> 
> I'm sure I'm being dumb, but where are we persisting the sidebar state
> between processes now, particularly for bookmarks loaded in the sidebar?

Not at all, it's jumping through a number of non-obvious hoops so it's worth stepping through that.

Which sidebar to reopen after a restart is remembered through the sidebarcommand attribute which is persisted via `document.persist("sidebar-box", "sidebarcommand");`.  This command id is used to fetch the relevant broadcaster element which has a sidebarurl attribute which points to the document to load in the <browser#sidebar> i.e. https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#130.

The web panel is a special case where the broadcaster points to web-panels.xul, which itself persists and loads the correct content URL through the cachedurl attribute: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/web-panels.xul#68 / https://dxr.mozilla.org/mozilla-central/source/browser/base/content/web-panels.js#68.
(In reply to :Gijs from comment #22)
> ::: browser/base/content/browser-sidebar.js:336
> (Diff revision 5)
> > -      }
> > +      // When loading a web page in the sidebar there is no title set on the
> > +      // broadcaster, as it is instead set by openWebPanel. Don't clear out
> > +      // the title in this case.
> 
> But are there other cases where there is no title? Should we set it to a
> default ("Sidebar") or something?

So this behavior is also confusing.  There are two main routes for a web page to load in the sidebar, however only one of them sets the title.

a) Call openWebPanel(title, uri) [0].  This happens from the Places UI when the bookmark is originally loaded [1], and note here that 'title' is not the title of the web page but the title in your db.
b) cachedurl is persisted and loaded when the sidebar starts out opened in a new window [2].  Since in this case we don't know the title from Places, we assume it will have been persisted in the sidebar.

This condition is handling case (b).  The problem is that we never explicitly set the sidebar title in this case, so if we reset it to a default value it will never get set correctly.  I could imagine setting the title to the pages document.title after a load, but that would incorrectly(?) override the Places DB title for case (a).

[0]: https://github.com/mozilla/gecko-dev/blob/e886d1846aae0a13ed810f96f2ea564e56933dd5/browser/base/content/browser.js#L5583
[1]: https://github.com/mozilla/gecko-dev/blob/9289913a09cad087624a3450de5844c6f2ac27fb/browser/components/places/PlacesUIUtils.jsm#L1051
[2]:  https://github.com/mozilla/gecko-dev/blob/e886d1846aae0a13ed810f96f2ea564e56933dd5/browser/base/content/web-panels.js#L76-L81
(In reply to Brian Grinstead [:bgrins] from comment #27)
> (In reply to :Gijs from comment #22)
> > ::: browser/base/content/browser-sidebar.js:336
> > (Diff revision 5)
> > > -      }
> > > +      // When loading a web page in the sidebar there is no title set on the
> > > +      // broadcaster, as it is instead set by openWebPanel. Don't clear out
> > > +      // the title in this case.
> > 
> > But are there other cases where there is no title? Should we set it to a
> > default ("Sidebar") or something?
> 
> So this behavior is also confusing.  There are two main routes for a web
> page to load in the sidebar, however only one of them sets the title.
> 
> a) Call openWebPanel(title, uri) [0].  This happens from the Places UI when
> the bookmark is originally loaded [1], and note here that 'title' is not the
> title of the web page but the title in your db.
> b) cachedurl is persisted and loaded when the sidebar starts out opened in a
> new window [2].  Since in this case we don't know the title from Places, we
> assume it will have been persisted in the sidebar.
> 
> This condition is handling case (b).  The problem is that we never
> explicitly set the sidebar title in this case, so if we reset it to a
> default value it will never get set correctly.  I could imagine setting the
> title to the pages document.title after a load, but that would
> incorrectly(?) override the Places DB title for case (a).

OK. This isn't a regression, right? So then let's just leave it as it is. :-)
(In reply to :Gijs from comment #28)
> OK. This isn't a regression, right? So then let's just leave it as it is. :-)

Correct :)
I'm running into an issue with the following test: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_988072_sidebar_events.js.  The issue is that it's testing two things:

1) the customize mode adding/removing of the sidebar toolbar button
2) the addon sdk's ability to add custom sidebars. The test doesn't directly use the SDK but it does the same thing the SDK does for adding sidebars.

The new sidebar switcher popup (in the sidebar header) doesn't display addon SDK sidebars (or sidebars in this test), because the SDK is expecting that the popup is the same thing as the View -> Sidebars menu.  But with the new design it's different elements (toolbarbuttons that observe the checked attribute of the View -> Sidebars menu). To be clear, SDK addons still show up in "View->Sidebars", just not in the new UI. And the new UI works with Web Extensions because that was updated in Bug 1365637.

The path forward here hinges on the question if we want addon SDK sidebars to show up in the new popup UI between now and when the SDK is deprecated. Andy, do you have an opinion on if we should do this?

If so, I believe we could change the new UI to dynamically build the new popup based on the view menu and update the test to look at the new popup. If not, then this test could be simplified quite a bit to only test case 1 above.
Flags: needinfo?(amckay)
There won't be any SDK add-ons on release in Firefox 57 and we plan to delete the SDK out of the tree soon after (see bug 1371065). I really don't want maintaining compat with the SDK to slow development down between now and then, so please don't worry about SDK add-ons showing up or not.
Flags: needinfo?(amckay)
(In reply to Andy McKay [:andym] from comment #31)
> There won't be any SDK add-ons on release in Firefox 57 and we plan to
> delete the SDK out of the tree soon after (see bug 1371065). I really don't
> want maintaining compat with the SDK to slow development down between now
> and then, so please don't worry about SDK add-ons showing up or not.

Thanks!  I'll update the test appropriately
Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

https://reviewboard.mozilla.org/r/143294/#review152552

::: browser/components/customizableui/CustomizableWidgets.jsm:602
(Diff revision 4)
> -    type: "view",
> -    viewId: "PanelUI-sidebar",
>      tooltiptext: "sidebar-button.tooltiptext2",
> -    onViewShowing(aEvent) {
> -      // Populate the subview with whatever menuitems are in the
> -      // sidebar menu. We skip menu elements, because the menu panel has no way
> +    onCommand(aEvent) {
> +      let win = aEvent.target.ownerGlobal;
> +      win.SidebarUI.toggle(win.SidebarUI.lastOpenedId || "viewBookmarksSidebar");

Interesting - why do you need to provide the default? Should this maybe be part of the toggle() method definition?

::: browser/components/customizableui/test/browser_sidebar_toggle.js:8
(Diff revision 4)
> -  let type = (isattr ? "on" : "") + event.type
> -  EVENTS[type]++;
> -};
> -
>  registerCleanupFunction(() => {
>    delete window.sawEvent;

This poor man's stubbing is dead now, I think, so this can be rm'd, too.

::: browser/components/customizableui/test/browser_sidebar_toggle.js:23
(Diff revision 4)
>    PanelUI.disableSingleSubviewPanelAnimations();
>  }
>  
>  function removeWidget() {
>    CustomizableUI.removeWidgetFromArea("sidebar-button");
>    PanelUI.enableSingleSubviewPanelAnimations();

I think we no longer need the animation enabling/disabling, so we can probably just have the addwidget/removewidget stuff directly in the async test function, without having helper functions that only contain 1 line of code themselves?

Alternatively, you could update the functions to be `maybeAddWidget` and -removeWidget, and only add/remove the widgets if they are not part of the default placements of the area (in a sense, future-proof the test for when the button will be there by default).

::: browser/components/customizableui/test/browser_sidebar_toggle.js:52
(Diff revision 4)
> -
> -  let sidebars = getSidebarList();
> -  let displayed = [...document.getElementById("PanelUI-sidebarItems").children];
> -  compareList(sidebars, displayed);
> -
> -  let subview = document.getElementById("PanelUI-sidebar");
> +  is(SidebarUI.currentID, "viewBookmarksSidebar", "Default sidebar selected");
> +  await SidebarUI.show("viewHistorySidebar");
> +
> +  await hideSidebar();
> +  await showSidebar();
> +  is(SidebarUI.currentID, "viewHistorySidebar", "Selected sidebar remembered");

Do we have other tests that check that we persist this correctly if you open a new window? And/or that it gets persisted across shutdowns (maybe just by verifying we store the right things in XUL storage, though marionette tests should let you test this 'properly') If not, can you file a followup to add those tests? Don't need to do it as part of photon / this project, but feels like something we should ideally have tests for given the increased prominence of the sidebar.
Attachment #8871824 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #39)
> ::: browser/components/customizableui/test/browser_sidebar_toggle.js:52
> (Diff revision 4)
> > -
> > -  let sidebars = getSidebarList();
> > -  let displayed = [...document.getElementById("PanelUI-sidebarItems").children];
> > -  compareList(sidebars, displayed);
> > -
> > -  let subview = document.getElementById("PanelUI-sidebar");
> > +  is(SidebarUI.currentID, "viewBookmarksSidebar", "Default sidebar selected");
> > +  await SidebarUI.show("viewHistorySidebar");
> > +
> > +  await hideSidebar();
> > +  await showSidebar();
> > +  is(SidebarUI.currentID, "viewHistorySidebar", "Selected sidebar remembered");
> 
> Do we have other tests that check that we persist this correctly if you open
> a new window? And/or that it gets persisted across shutdowns (maybe just by
> verifying we store the right things in XUL storage, though marionette tests
> should let you test this 'properly') If not, can you file a followup to add
> those tests? Don't need to do it as part of photon / this project, but feels
> like something we should ideally have tests for given the increased
> prominence of the sidebar.

Good idea. I swear I've had times where the state is not fully persisted (for instance, it might remember which sidebar is opened but not the title or vice versa), but I've been unable to find STR.  Putting a test in place might surface that, or at least give us a place to put a regression test given STR
(In reply to :Gijs from comment #39) 
> ::: browser/components/customizableui/test/browser_sidebar_toggle.js:8
> (Diff revision 4)
> > -  let type = (isattr ? "on" : "") + event.type
> > -  EVENTS[type]++;
> > -};
> > -
> >  registerCleanupFunction(() => {
> >    delete window.sawEvent;
> 
> This poor man's stubbing is dead now, I think, so this can be rm'd, too.
> 
> ::: browser/components/customizableui/test/browser_sidebar_toggle.js:23
> (Diff revision 4)
> >    PanelUI.disableSingleSubviewPanelAnimations();
> >  }
> >  
> >  function removeWidget() {
> >    CustomizableUI.removeWidgetFromArea("sidebar-button");
> >    PanelUI.enableSingleSubviewPanelAnimations();
> 
> I think we no longer need the animation enabling/disabling, so we can
> probably just have the addwidget/removewidget stuff directly in the async
> test function, without having helper functions that only contain 1 line of
> code themselves?
> 
> Alternatively, you could update the functions to be `maybeAddWidget` and
> -removeWidget, and only add/remove the widgets if they are not part of the
> default placements of the area (in a sense, future-proof the test for when
> the button will be there by default).

Another good point. I will call resetCustomization() in the cleanup function instead of removeWidget, and call `CustomizableUI.addWidgetToArea("sidebar-button", "nav-bar");` at the start (which doesn't seem to complain if the toolbarbutton is already in there).
Blocks: 1372398
(In reply to :Gijs from comment #39)
> Comment on attachment 8871824 [details]
> Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;
> 
> https://reviewboard.mozilla.org/r/143294/#review152552
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm:602
> (Diff revision 4)
> > -    type: "view",
> > -    viewId: "PanelUI-sidebar",
> >      tooltiptext: "sidebar-button.tooltiptext2",
> > -    onViewShowing(aEvent) {
> > -      // Populate the subview with whatever menuitems are in the
> > -      // sidebar menu. We skip menu elements, because the menu panel has no way
> > +    onCommand(aEvent) {
> > +      let win = aEvent.target.ownerGlobal;
> > +      win.SidebarUI.toggle(win.SidebarUI.lastOpenedId || "viewBookmarksSidebar");
> 
> Interesting - why do you need to provide the default? Should this maybe be
> part of the toggle() method definition?

The next patch in the series does this (it's intended to be folded in but I kept it separate here to make it easier to review).  The distinction between currentID and lastOpenedId can be a little confusing so I wanted write down what I've gathered:

* currentID is a getter based on the sidebarcommand attribute on the box.  This attribute is set during show() and also persisted in the document on uninit. The attribute is cleared out on hide().
* lastOpenedId is set immediately after the sidebarcommand attribute in show() but it's not cleared out on hide and it also isn't persisted across restarts

The difference is important for toggle() because we don't want to use currentID (since it will be null whenever the sidebar is hidden and therefore doesn't make any sense as the default value). As for persistence, if the attribute was persisted on the document then show() will have been called at startup before toggle is ever called, bringing the two properties into sync.
Comment on attachment 8876950 [details]
Bug 1360282 - TO FOLD INTO PREVIOUS PATCH;

https://reviewboard.mozilla.org/r/148270/#review152840

Can you add the documentation about currentID vs lastOpenedId next to the added definition of lastOpenedId?

Also, I love (I mean, no, that other thing) that we have both "Id" and "ID" as pre-existing spellings here. And that we have two kind of similar but not quite the same properties. Followup to tighten that up post-57 (so we don't break any add-ons) ? It sounds like this should really just be 1 property, or maybe 1 property plus a bool indicating whether or not the sidebar was open when we quit.

::: browser/base/content/browser-sidebar.js:283
(Diff revision 1)
>     */
> -  toggle(commandID = this.currentID) {
> +  toggle(commandID) {
> +    // First priority for a default value is this.lastOpenedId which is set during show()
> +    // and not reset in hide(), unlike currentID. If show() hasn't been called or the command
> +    // doesn't exist anymore, then fallback to a default sidebar.
> +    commandID = commandID || this.lastOpenedId;

Can we do this as the default, ie define this method as:

```js
toggle(commandID = this.lastOpenedId);
```

or does that not work for some reason?
Attachment #8876950 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8876950 - Attachment is obsolete: true
Depends on: 1372601
Blocks: 1372605
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1519a696f6f
Always call `show` to open the sidebar, even when it's called from startDelayedLoad;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/447cffa065b1
Use querySelectorAll instead of getElementsByAttribute and a condition;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6408339e572e
Remove _setVisibility helper now that the sidebar is always shown through show();r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f246c7bba040
Use the existing title setter for SidebarUI instead of changing the value directly;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/114fb8a5f56c
Remove the popup that opens from the sidebar toolbar button;r=Gijs
Iteration: --- → 56.1 - Jun 26
Depends on: 1372742
Depends on: 1372954
I have reproduced this bug with Nightly 55.0a1 (2017-04-27) on Windows 8.1 (64 bit).

This bug's fix is verified on Latest Nightly  56.0a1.

Build ID : 20170615030208
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170614]
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Blocks: 1387512
Depends on: 1394207
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: