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

VERIFIED FIXED in Firefox 55

Status

P1
normal
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: andy+bugzilla, Assigned: mixedpuppy)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure] triaged)

Attachments

(1 attachment)

(Reporter)

Description

2 years 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

2 years ago
Assignee: nobody → mixedpuppy
Whiteboard: investigating
(Assignee)

Updated

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

Comment 2

2 years 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

2 years 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
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8868720 - Flags: review?(bgrinstead)
(Assignee)

Comment 9

2 years 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

2 years 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)
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(Assignee)

Comment 13

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8868720 - Flags: review?(aswan)

Comment 22

2 years 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
Comment hidden (mozreview-request)

Comment 25

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ad409d79405
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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 years 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 years 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)
(Assignee)

Updated

2 years ago
Depends on: 1394947

Updated

8 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.