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

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P1
normal
VERIFIED FIXED
3 months ago
15 days ago

People

(Reporter: andym, Assigned: mixedpuppy)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
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.

Updated

3 months ago
Assignee: nobody → mixedpuppy
Whiteboard: investigating
(Assignee)

Updated

3 months ago
Blocks: 1355324
Priority: -- → P2
Whiteboard: investigating → triaged
Blocks: 1360282
Blocks: 1353421
Whiteboard: triaged → [photon-structure] triaged
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
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.
Attachment #8868720 - Flags: feedback?(bgrinstead)
(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?
Attachment #8868720 - Flags: feedback?(bgrinstead) → feedback+
Nit: typo in the commit message (dropdwon)
(Assignee)

Comment 5

3 months ago
(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?
(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)
The sidebar styles are now in browser/themes/shared/sidebars.inc.css

Updated

3 months ago
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8868720 - Flags: review?(bgrinstead)
(Assignee)

Comment 9

3 months ago
r? bgrins for browser/sidebar changes, mattw for webext sidebar changes, aswan for rubber stamp.
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
Attachment #8868720 - Flags: review?(bgrinstead) → review?(gijskruitbosch+bugs)

Comment 11

3 months ago
mozreview-review
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?
Attachment #8868720 - Flags: review?(gijskruitbosch+bugs) → review-
(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?
Flags: needinfo?(gijskruitbosch+bugs)

Updated

3 months ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(Assignee)

Comment 13

3 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 15

3 months ago
I decided to just add the button directly rather than replicating the element copy stuff that the old button did.
Comment hidden (mozreview-request)

Comment 17

3 months ago
mozreview-review
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!
Attachment #8868720 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 18

3 months ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 19

3 months ago
mozreview-review
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.
Attachment #8868720 - Flags: review?(mwein) → review+
(Assignee)

Comment 20

3 months ago
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.
Attachment #8868720 - Flags: review?(aswan)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8868720 - Flags: review?(aswan)

Comment 22

3 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69a878129f88
place WE sidebars into the photon sidebar dropdown, r=Gijs,mattw
Backed out in https://hg.mozilla.org/integration/autoland/rev/bc2d16e4e1ad for https://treeherder.mozilla.org/logviewer.html#?job_id=102895846&repo=autoland
Comment hidden (mozreview-request)

Comment 25

3 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7ad409d79405
place WE sidebars into the photon sidebar dropdown, r=Gijs,mattw

Comment 26

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ad409d79405
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

3 months ago
Iteration: --- → 55.7 - Jun 12
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!
Flags: needinfo?(gijskruitbosch+bugs)

Comment 28

2 months ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
(Reporter)

Comment 29

2 months ago
As an example: https://addons.mozilla.org/en-US/firefox/addon/sidebar-tabs-webextension/ and FYI it works great.
Looks like it works now for Windows, Mac, and Ubuntu, so changing to VERIFIED. 

Thanks for the help, Andy and Gijs.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.