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

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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?

Comment 2

[Aaron Benson]

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

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

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

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 6

[Brian Grinstead [:bgrins]]

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 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 8

[Brian Grinstead [:bgrins]]

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)

Comment 9

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

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 10

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

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! :-)

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 21

[Brian Grinstead [:bgrins]]

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 22

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

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?

Comment 23

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

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

https://reviewboard.mozilla.org/r/143254/#review150686

Comment 24

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

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

Comment 25

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

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

Comment 26

[Brian Grinstead [:bgrins]]

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

Comment 27

[Brian Grinstead [:bgrins]]

(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

Comment 28

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

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

Comment 29

[Brian Grinstead [:bgrins]]

(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 :)

Comment 30

[Brian Grinstead [:bgrins]]

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.

Comment 31

[Andy McKay]

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.

Comment 32

[Brian Grinstead [:bgrins]]

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

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 38

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 39

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

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.

Comment 40

[Brian Grinstead [:bgrins]]

(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

Comment 41

[Brian Grinstead [:bgrins]]

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

Comment 42

[Brian Grinstead [:bgrins]]

Attachment: Bug 1360282 - TO FOLD INTO PREVIOUS PATCH;

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

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

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

Comment 44

[Brian Grinstead [:bgrins]]

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 45

[Brian Grinstead [:bgrins]]

Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 46

[Brian Grinstead [:bgrins]]

Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 47

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 48

[Brian Grinstead [:bgrins]]

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

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

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?

Comment 50

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142960/diff/7-8/

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 52

[Brian Grinstead [:bgrins]]

Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 53

[Brian Grinstead [:bgrins]]

Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 54

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 55

[Brian Grinstead [:bgrins]]

Comment on attachment 8871495 [details]
Bug 1360282 - Always call `show` to open the sidebar, even when it's called from startDelayedLoad;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142960/diff/8-9/

Comment 56

[Brian Grinstead [:bgrins]]

Comment on attachment 8871822 [details]
Bug 1360282 - Use querySelectorAll instead of getElementsByAttribute and a condition;

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

Comment 57

[Brian Grinstead [:bgrins]]

Comment on attachment 8871821 [details]
Bug 1360282 - Remove _setVisibility helper now that the sidebar is always shown through show();

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

Comment 58

[Brian Grinstead [:bgrins]]

Comment on attachment 8871823 [details]
Bug 1360282 - Use the existing title setter for SidebarUI instead of changing the value directly;

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

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8871824 [details]
Bug 1360282 - Remove the popup that opens from the sidebar toolbar button;

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

Comment 60

[Pulsebot]

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

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1519a696f6f
https://hg.mozilla.org/mozilla-central/rev/447cffa065b1
https://hg.mozilla.org/mozilla-central/rev/6408339e572e
https://hg.mozilla.org/mozilla-central/rev/f246c7bba040
https://hg.mozilla.org/mozilla-central/rev/114fb8a5f56c

Comment 62

[Azmina]

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

Comment 63

[u554753]

Verified on Windows, Mac, and Ubuntu.