1365637 - Sidebar doesn't show up in the new Photon sidebar selector

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Andy McKay]

I've got a sidebar extension. It shows up in the sidebar button, but it doesn't show up in the new sidebard selector that's in Nightly.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

@bgrins:  note that I am inserting prior to the separator.  a) should we add an id to that, and b) I'm assuming this is a fine location to insert.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Comment on attachment 8868720 [details]
> Bug 1365637 place WE sidebars into the photon sidebar dropdwon,
> 
> @bgrins:  note that I am inserting prior to the separator.  a) should we add
> an id to that, and b) I'm assuming this is a fine location to insert.

Yes, after the built in sidebars and before the separator.  The only difference is that there should be an addition separator before the sidebar extensions - see the third image here: https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/231191185_Explainer_-_Sidebar.

I was considering modifying browser.xul to look like so:

    <toolbarbutton id="sidebar-switcher-tabs" .../>
    <toolbarseparator/>
    <vbox id="sidebar-extensions"></vbox>
    <toolbarseparator/>
    <toolbarbutton label="&sidebarCloseButton.tooltip;" .../>

Then append the toolbarbutton into #sidebar-extensions, and make the sibling toolbarseparator after #sidebar-extensions conditional on it not being empty.   Maybe something like: `sidebar-extensions:empty + toolbarseparator { display: none; }`.  Does that make sense?

Comment 4

[Brian Grinstead [:bgrins]]

Nit: typo in the commit message (dropdwon)

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> I was considering modifying browser.xul to look like so:
> 
>     <toolbarbutton id="sidebar-switcher-tabs" .../>
>     <toolbarseparator/>
>     <vbox id="sidebar-extensions"></vbox>
>     <toolbarseparator/>
>     <toolbarbutton label="&sidebarCloseButton.tooltip;" .../>
> 
> Then append the toolbarbutton into #sidebar-extensions, and make the sibling
> toolbarseparator after #sidebar-extensions conditional on it not being
> empty.   Maybe something like: `sidebar-extensions:empty + toolbarseparator
> { display: none; }`.  Does that make sense?

IMO that would be much better than managing the insertion/removal of a separator in the WE code.  Do you want to do that before landing this bug?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > I was considering modifying browser.xul to look like so:
> > 
> >     <toolbarbutton id="sidebar-switcher-tabs" .../>
> >     <toolbarseparator/>
> >     <vbox id="sidebar-extensions"></vbox>
> >     <toolbarseparator/>
> >     <toolbarbutton label="&sidebarCloseButton.tooltip;" .../>
> > 
> > Then append the toolbarbutton into #sidebar-extensions, and make the sibling
> > toolbarseparator after #sidebar-extensions conditional on it not being
> > empty.   Maybe something like: `sidebar-extensions:empty + toolbarseparator
> > { display: none; }`.  Does that make sense?
> 
> IMO that would be much better than managing the insertion/removal of a
> separator in the WE code.  Do you want to do that before landing this bug?

I think it'd make the most sense to add it into this bug (either in this patch or in a series)

Comment 7

[Brian Grinstead [:bgrins]]

The sidebar styles are now in browser/themes/shared/sidebars.inc.css

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 9

[Shane Caraveo (:mixedpuppy)]

r? bgrins for browser/sidebar changes, mattw for webext sidebar changes, aswan for rubber stamp.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

Forwarding my portion of the review to Gijs for a second opinion, since that part is code from my comment

Comment 11

[:Gijs (he/him)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

https://reviewboard.mozilla.org/r/140314/#review144052

r- for now - what did I miss? :-)

::: commit-message-85e5d:1
(Diff revision 2)
> +Bug 1365637 place WE sidebars into the photon sidebar dropdwon, r?mattw,aswan

dropdown

::: browser/components/extensions/ext-sidebarAction.js:153
(Diff revision 2)
> -    menuitem.setAttribute("class", "menuitem-iconic webextension-menuitem");
> +    menuitem.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
>  
>      this.setMenuIcon(menuitem, details);
>  
>      document.getElementById("mainBroadcasterSet").appendChild(broadcaster);
> -    document.getElementById("viewSidebarMenu").appendChild(menuitem);
> +    document.getElementById("sidebar-extensions").appendChild(menuitem);

I feel like I've missed some discussion. Why are we no longer inserting into the main (View > Sidebars) menu?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to :Gijs from comment #11)
> Comment on attachment 8868720 [details]
> Bug 1365637 place WE sidebars into the photon sidebar dropdwon,
> 
> https://reviewboard.mozilla.org/r/140314/#review144052
> 
> r- for now - what did I miss? :-)
> 
> ::: commit-message-85e5d:1
> (Diff revision 2)
> > +Bug 1365637 place WE sidebars into the photon sidebar dropdwon, r?mattw,aswan
> 
> dropdown
> 
> ::: browser/components/extensions/ext-sidebarAction.js:153
> (Diff revision 2)
> > -    menuitem.setAttribute("class", "menuitem-iconic webextension-menuitem");
> > +    menuitem.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
> >  
> >      this.setMenuIcon(menuitem, details);
> >  
> >      document.getElementById("mainBroadcasterSet").appendChild(broadcaster);
> > -    document.getElementById("viewSidebarMenu").appendChild(menuitem);
> > +    document.getElementById("sidebar-extensions").appendChild(menuitem);
> 
> I feel like I've missed some discussion. Why are we no longer inserting into
> the main (View > Sidebars) menu?

Good catch - I was thinking of this only affecting the popup being used by the toolbarbutton menu item which is being removed in Bug 1360282.  I didn't realize this was the same popup used in View->Sidebars.  I guess we need to insert the item in both places now?

Comment 13

[Shane Caraveo (:mixedpuppy)]

Ah, I forgot what the toolbarbutton actually did.  It copies the elements from the view->sidebars menu.  Should we do the same for the new dropdown?

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

I decided to just add the button directly rather than replicating the element copy stuff that the old button did.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 17

[:Gijs (he/him)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

https://reviewboard.mozilla.org/r/140314/#review144564

Code LGTM, thanks!

Comment 18

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> I decided to just add the button directly rather than replicating the
> element copy stuff that the old button did.

I think this is correct.

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Good catch - I was thinking of this only affecting the popup being used by
> the toolbarbutton menu item which is being removed in Bug 1360282.  I didn't
> realize this was the same popup used in View->Sidebars.  I guess we need to
> insert the item in both places now?

Yep.

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

https://reviewboard.mozilla.org/r/140314/#review147660

::: browser/components/extensions/ext-sidebarAction.js:197
(Diff revision 4)
>      let title = tabData.title || this.extension.name;
>      let menu = document.getElementById(this.menuId);
>      if (!menu) {
>        menu = this.createMenuItem(window, tabData);
>      }
> +    let button = document.getElementById(this.buttonId);

Please put this line right before `button` is used.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

Given review from mattw and Gijs, I don't think a 3rd is necessary.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69a878129f88
place WE sidebars into the photon sidebar dropdown, r=Gijs,mattw

Comment 23

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/bc2d16e4e1ad for https://treeherder.mozilla.org/logviewer.html#?job_id=102895846&repo=autoland

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8868720 [details]
Bug 1365637 place WE sidebars into the photon sidebar dropdown,

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ad409d79405
place WE sidebars into the photon sidebar dropdown, r=Gijs,mattw

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ad409d79405

Comment 27

[u554753]

Gijs, I was trying to look at some of the mockups to verify this bug, but they seem to be invalid links now. Any suggested STR to be able to verify this? Thanks!

Comment 28

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #27)
> Gijs, I was trying to look at some of the mockups to verify this bug, but
> they seem to be invalid links now. Any suggested STR to be able to verify
> this? Thanks!

https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252088 should work (but it's broken for me right now, not sure if that will be fixed soon)

More generally, I think you should be able to test with a webextension that provides a sidebar, that its sidebar shows up in the menu that appears if you click the header of the sidebar (e.g. when opening the bookmarks or history or synced tabs sidebar), and that the menuitem works in that it loads the right sidebar content into the sidebar. Does that help?

Comment 29

[Andy McKay]

As an example: https://addons.mozilla.org/en-US/firefox/addon/sidebar-tabs-webextension/ and FYI it works great.

Comment 30

[u554753]

Looks like it works now for Windows, Mac, and Ubuntu, so changing to VERIFIED. 

Thanks for the help, Andy and Gijs.