It's a header in the sidebar that lets you switch between different sidebar views.

FWIW, the panelUI.js changes in https://reviewboard.mozilla.org/r/132112/diff/2/ should help with the shortcut stuff (basically, set a "shortcut" attribute and you'll get an ::after on the toolbar text that gets styled as a shortcut thingy).

Clickable Prototype: https://mozilla.invisionapp.com/share/XRBHN5SWD#/229252085_Sidebar_-_Bookmarks Spec Sheet: https://mozilla.invisionapp.com/share/XSBHN694Y#/231191185_Explainer_-_Sidebar

Aaron, I've implemented the top part of the spec sheet (switcher control and UI updates for the sidebar header). I'd like some UI/UX feedback on the progress so far if you can test it out. Here's a binary download you can run for testing: https://archive.mozilla.org/pub/firefox/try-builds/bgrinstead@mozilla.com-9379528b9a6a35f57faf1206257d65ba736af3f7/try-macosx64/firefox-55.0a1.en-US.mac.dmg. Instructions: * System preferences -> Security & Privacy -> Change "Allow apps downloaded from" to "Anywhere" * Don't drag it into your applications folder, just double click the Nightly app from the window that opens after running it * Note that when loading if it asks you to pick a profile then I'd suggest you make a new one * Go into about:config and flip browser.photon.structure.enabled = true * Close and reopen the browser with the same profile

A note about the alignment of the icons in the panel with the anchor icon for the panel. This isn't how panels typically line up - they are anchored based on all of the content (including the native checkbox). Is this a UI pattern used elsewhere in photon? If we did use some magic numbers on each platform to get the checkbox scooted to the left enough to line up the icons, they still end up out of alignment in scenarios when the window is all the way to the right and the sidebar is on the right. I've attached a screenshot of this scenario - if the window is all the way on the right of the screen there is no way to line up the icons without moving the popup content off the screen. A similar situation can happen if the window is all the way to the left and the sidebar is on the left.

(In reply to Brian Grinstead [:bgrins] from comment #9) > Created attachment 8863060 [details] > icon-sidebar-right.png > > A note about the alignment of the icons in the panel with the anchor icon > for the panel. This isn't how panels typically line up - they are anchored > based on all of the content (including the native checkbox). Is this a UI > pattern used elsewhere in photon? > > If we did use some magic numbers on each platform to get the checkbox > scooted to the left enough to line up the icons, they still end up out of > alignment in scenarios when the window is all the way to the right and the > sidebar is on the right. I've attached a screenshot of this scenario - if > the window is all the way on the right of the screen there is no way to line > up the icons without moving the popup content off the screen. A similar > situation can happen if the window is all the way to the left and the > sidebar is on the left. That makes sense. It's not necessary to create a special rule for how the context menus work; that was some wishful designer thinking on my part :)

The latest changes move the themes out into a new shared file and includes support for Windows. There's also a simple test case for the switcher.

Requesting review/feedback - feel free to redirect. There are a few known issues: 1) When selecting the same sidebar that's already picked from the popup it closes the sidebar. Probably a carryover from the toolbar popup behavior, but should be fine to change ahead of Bug 1360282. 2) The #sidebar-switcher-target element doesn't receive focus.. it should probably be converted away from a box but I haven't been able to figure out how to get the styling right on a toolbarbutton or similar yet 3) Not necessarily something that needs changing, but there's a TODO around figuring out why many of the selectors are duplicated as both an ID and class - i.e. `.sidebar-title, #sidebar-title` Also, note that it's using a panelmultiview even though there's only one view so that we can get identical styling as the main hamburger popup. I'd have to look through more closely across platforms, but it might not be too much CSS to duplicate if we wanted to put a vbox directly as a child of the panel.

Screenshot in ubuntu - old on right and new on left

Comment on attachment 8862170 [details] Bug 1355324 - Create a popup to switch between sidebars from the sidebar header; https://reviewboard.mozilla.org/r/134156/#review139682 ::: browser/themes/linux/browser.css (Diff revision 9) > } > > /* Content area */ > -#sidebar { > - background-color: Window; > -} Is there a reason why you removed this rule on Linux but kept it on Windows? ::: browser/themes/shared/sidebar.inc.css:3 (Diff revision 9) > + > +#sidebar-box { > + -moz-context-properties: fill; Although -moz-context-properties gets inherited, you should for performance reasons set it more specifically on the elements that need it. You'll also need to specify a fill color, otherwise the SVGs just use black. fill: currentColor will work well, I think. ::: browser/themes/shared/sidebar.inc.css:9 (Diff revision 9) > +} > + > +.sidebar-header, > +#sidebar-header { > + padding: 4px; > + background-color: #F2F2F2; This needs to be a system color such as Window or -moz-dialog. ::: browser/themes/shared/sidebar.inc.css:15 (Diff revision 9) > + text-shadow: none; > +} > + > +.sidebar-splitter { > + -moz-appearance: none; > + border: 0 solid #ccc; This should be a system color too. ThreeDShadow maybe? ::: browser/themes/shared/sidebar.inc.css:39 (Diff revision 9) > +#sidebar-title { > + margin: 0; > + padding: 0; > + padding-inline-start: 8px; > + padding-inline-end: 4px; > + color: #051126; This needs to be a system color. ::: browser/themes/shared/sidebar.inc.css:40 (Diff revision 9) > + margin: 0; > + padding: 0; > + padding-inline-start: 8px; > + padding-inline-end: 4px; > + color: #051126; > + font-size: 13px; Why are you hardcoding a font-size here? ::: browser/themes/shared/sidebar.inc.css:41 (Diff revision 9) > + padding: 0; > + padding-inline-start: 8px; > + padding-inline-end: 4px; > + color: #051126; > + font-size: 13px; > + font-family: 'Segoe UI', -apple-system, sans-serif; Ugh, why specify a font here? Why isn't this keeping the default font like we do everywhere else in chrome? ::: browser/themes/shared/sidebar.inc.css:65 (Diff revision 9) > +} > + > +#sidebar-box #sidebar-switcher-target:hover, > +#sidebar-box #sidebarMenu-popup:not([hidden]) ~ #sidebar-switcher-target, > +#sidebar-close:hover { > + background: rgba(204, 204, 204, 0.6); This looks like it won't work well with high contrast themes. ::: browser/themes/shared/sidebar.inc.css:80 (Diff revision 9) > + padding-inline-start: 16px; > +} > +#sidebarMenu-popup .subviewbutton-iconic > .toolbarbutton-text { > + padding-inline-start: 0; > +} > +%endif Please move this to osx/browser.css after importing sidebar.inc.css.

For the colors / font questions, I've been working off of https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/231191185_Explainer_-_Sidebar, and have the understanding that those properties should be the same on all platforms.

(In reply to Dão Gottwald [::dao] from comment #23) > ::: browser/themes/shared/sidebar.inc.css:80 > (Diff revision 9) > > + padding-inline-start: 16px; > > +} > > +#sidebarMenu-popup .subviewbutton-iconic > .toolbarbutton-text { > > + padding-inline-start: 0; > > +} > > +%endif > > Please move this to osx/browser.css after importing sidebar.inc.css. This is ifndef, so the rules apply to linux/windows. I originally had it copied in both places but moved it here to get rid of duplication. Happy to pull it back out to those files if you think it's best.

(In reply to Dão Gottwald [::dao] from comment #23) > Comment on attachment 8862170 [details] > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > header; > > https://reviewboard.mozilla.org/r/134156/#review139682 > > ::: browser/themes/linux/browser.css > (Diff revision 9) > > } > > > > /* Content area */ > > -#sidebar { > > - background-color: Window; > > -} > > Is there a reason why you removed this rule on Linux but kept it on Windows? Ah, I didn't mean to remove that from linux. will revert that (this patch is only addressing the header) > ::: browser/themes/shared/sidebar.inc.css:3 > (Diff revision 9) > > + > > +#sidebar-box { > > + -moz-context-properties: fill; > > Although -moz-context-properties gets inherited, you should for performance > reasons set it more specifically on the elements that need it. > > You'll also need to specify a fill color, otherwise the SVGs just use black. > fill: currentColor will work well, I think. OK, will do

Talked with shorlander about the font stack.. going to get rid of it

(In reply to Brian Grinstead [:bgrins] from comment #21) > Requesting review/feedback - feel free to redirect. There are a few known > issues: > > 2) The #sidebar-switcher-target element doesn't receive focus.. it should > probably be converted away from a box but I haven't been able to figure out > how to get the styling right on a toolbarbutton or similar yet This issue is now fixed in the latest version of the patch

Comment on attachment 8862170 [details] Bug 1355324 - Create a popup to switch between sidebars from the sidebar header; https://reviewboard.mozilla.org/r/134156/#review139802 Just about enough comments not to r+ immediately, but this looks really good! One related thing that I picked up when testing on Win7 and Win10 - the background of the sidebars is still this Win7-y blue. This becomes more obvious with this patch because it looks ugly next to the kind-of-similar-but-not-the-same grey background on the header, and the win7-style blue search icon looks odd (and blurry, on hidpi) next to the new crisp SVG ones. Another thing I noticed is that there are no borders around the search field in the synced tab sidebar (on either Windows 7 or Windows 10), and it's not obvious there's a search field there at all. I don't think either of these things desperately needs fixing in this bug, but if we don't address it here we should make sure a followup gets filed for it. ::: browser/base/content/browser-sidebar.js:59 (Diff revision 12) > + if (this._switcherPanel.state == "open") { > + this.hideSwitcherPanel(); > + } else if (this._switcherPanel.state == "closed") { > + this.showSwitcherPanel(); > + } As written this does nothing if you hit it while the panel is opening/closing (you can check by doing quick double-clicks on the anchor - depending on how quick you are, it either opens and then closes, or just opens and treats the second click as a no-op). I think we should probably ensure that the behaviour is consistent in that, if you click when we're still opening, we close the panel, and if you click when we're hiding, we open it. ::: browser/base/content/browser-sidebar.js:67 (Diff revision 12) > + return new Promise(resolve => { > + this._switcherPanel.addEventListener("popuphidden", () => { > + resolve(); > + }, {once: true}); > + this._switcherPanel.hidePopup(); > + }); I don't see us ever using the promise here? ::: browser/base/content/browser-sidebar.js:87 (Diff revision 12) > + this._switcherPanel.addEventListener("popupshown", () => { > + resolve(); > + }, {once: true}); If we hide the popup while showing it (before popupshown fires), this never resolves, I think? I also think that this never gets used except in tests, so I wonder if we should just add the promise stuff as a helper in the test and not bother with it here... ::: browser/base/content/browser.xul:1091 (Diff revision 12) > <deck id="content-deck" flex="1"> > <hbox flex="1" id="browser"> > <vbox id="browser-border-start" hidden="true" layer="true"/> > <vbox id="sidebar-box" hidden="true" class="chromeclass-extrachrome"> > <sidebarheader id="sidebar-header" align="center"> > - <label id="sidebar-title" persist="value" flex="1" crop="end" control="sidebar"/> > + <panel id="sidebarMenu-popup" My experience is that adding popups as children of other nodes leads to pain. Specifically pain like in bug 1227540. In this particular case, I don't see a lot of upsides - I don't think we need to keep the panel here because of styling. Can you add it to #mainPopupSet instead? ::: browser/base/content/browser.xul:1098 (Diff revision 12) > + role="group" > + type="arrow" > + hidden="true" > + flip="slide" > + position="bottomcenter topleft" > + noautofocus="true" Do we need the noautofocus? ::: browser/base/content/browser.xul:1100 (Diff revision 12) > + <panelmultiview id="sidebarMenu-multiView" mainViewId="sidebarMenu-mainView"> > + <panelview id="sidebarMenu-mainView" class="cui-widget-panelview PanelUI-subView"> > + <vbox class="panel-subview-body"> I don't believe we're planning to ever show subviews in here, so I think you can omit the <panelmultiview> and <panelview> elements. Unless that makes the styling story a lot more difficult? ::: browser/base/content/browser.xul:1133 (Diff revision 12) > + oncommand="SidebarUI.hide()"/> > + </vbox> > + </panelview> > + </panelmultiview> > + </panel> > + <toolbarbutton id="sidebar-switcher-target" flex="1" class="tabbable"> On Windows, after clicking an item, I see focus rings on the button. What's causing this? There shouldn't be focus rings unless I start using [Tab] to move focus around. ::: browser/base/content/browser.xul:1139 (Diff revision 12) > + <image id="sidebar-icon"/> > + <label id="sidebar-title" persist="value" crop="end" control="sidebar"/> > + <image id="sidebar-switcher-arrow"/> > + <spacer flex="1"/> > + </toolbarbutton> > <image id="sidebar-throbber"/> You seem to have left the markup but, AFAICT, are removing all the styling. Should we remove the markup as well? Why are we removing the styling? There's now no more feedback about pages that load slowly, which is a bit sad. :-( ::: browser/base/content/test/sidebar/browser.ini:3 (Diff revision 12) > +[DEFAULT] > + > +[browser_sidebar_switcher.js] Can you also move browser_bug409481.js while we're here? ::: browser/base/content/test/sidebar/browser_sidebar_switcher.js:5 (Diff revision 12) > + > +add_task(function* () { > + // If a sidebar is already open, close it. > + if (!document.getElementById("sidebar-box").hidden) { > + info("Unexpected sidebar found - a previous test failed to cleanup correctly"); Should this ok(false, ...) ? ::: browser/base/content/test/sidebar/browser_sidebar_switcher.js:15 (Diff revision 12) > + switcherPromise = Promise.all([ > + BrowserTestUtils.waitForEvent(window, "SidebarFocused"), > + BrowserTestUtils.waitForEvent(sidebarPopup, "popuphidden"), > + ]); 3 repetitions... should we stick this in a helper function? :-) ::: browser/themes/shared/sidebar.inc.css:1 (Diff revision 12) > + Nit: can you add a % commented out license header? ::: browser/themes/shared/sidebar.inc.css:42 (Diff revision 12) > +#sidebar-title { > + margin: 0; > + padding: 0; > + padding-inline-start: 8px; > + padding-inline-end: 4px; > + font-size: var(--title-font-size); We don't normally set font size so that we work correctly when people modify default Windows font sizes. If we do really need to set a font-size, setting it in em would be preferable, I think. ::: browser/themes/shared/sidebar.inc.css:56 (Diff revision 12) > +} > + > +#sidebar-close { > + -moz-appearance: none; > + -moz-context-properties: fill; > + fill: var(--icon-fill); This is hardcoded so it doesn't work in the dark compact theme (on Windows, at least). Same for the icon displayed in the header and the down arrow. Did currentColor, as Dão suggested, not work? ::: browser/themes/shared/sidebar/bookmark-16-outline.svg:1 (Diff revision 12) > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> For all of these, can we drop the -16- infix, add a license header, and drop the viewBox attribute? ::: browser/themes/shared/sidebar/bookmark-16-outline.svg:1 (Diff revision 12) > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> I guess more generally... it's a bit unfortunate that because we're all working on different bits (across photon, not just structure), it's hard to see how to best package things such that paths are relatively consistent and we reuse identical icons. All of these icons look like they're essentially the same icons we'll be using for toolbar buttons and/or menu panel items. So I'm not sure sidebar/ is the best place for them. Equally, I'm not sure if it's premature to create a single icons/ folder or something like that (plus that then quickly becomes a dumping ground). Let's leave it like this for now, and if we find we need/want to reuse things, I guess we can unify things then.

(In reply to :Gijs from comment #32) > Comment on attachment 8862170 [details] > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > header; > > https://reviewboard.mozilla.org/r/134156/#review139802 > > Just about enough comments not to r+ immediately, but this looks really good! > > One related thing that I picked up when testing on Win7 and Win10 - the > background of the sidebars is still this Win7-y blue. This becomes more > obvious with this patch because it looks ugly next to the > kind-of-similar-but-not-the-same grey background on the header, and the > win7-style blue search icon looks odd (and blurry, on hidpi) next to the new > crisp SVG ones. > > Another thing I noticed is that there are no borders around the search field > in the synced tab sidebar (on either Windows 7 or Windows 10), and it's not > obvious there's a search field there at all. > > I don't think either of these things desperately needs fixing in this bug, > but if we don't address it here we should make sure a followup gets filed > for it. I was thinking anything below the headers could be handled in the corresponding sidebar update bug (Bug 1355326, Bug 1355328, and Bug 1355330). But I'm open to add that in here if it looks too bad. > ::: browser/base/content/browser-sidebar.js:59 > (Diff revision 12) > > + if (this._switcherPanel.state == "open") { > > + this.hideSwitcherPanel(); > > + } else if (this._switcherPanel.state == "closed") { > > + this.showSwitcherPanel(); > > + } > > As written this does nothing if you hit it while the panel is > opening/closing (you can check by doing quick double-clicks on the anchor - > depending on how quick you are, it either opens and then closes, or just > opens and treats the second click as a no-op). I think we should probably > ensure that the behaviour is consistent in that, if you click when we're > still opening, we close the panel, and if you click when we're hiding, we > open it. OK, this may or may not also fix the need for noautofocus (which I added for Linux due to clicking the anchor when the popup was opening causing the popup to briefly close but reopen. > ::: browser/base/content/browser-sidebar.js:67 > (Diff revision 12) > > + return new Promise(resolve => { > > + this._switcherPanel.addEventListener("popuphidden", () => { > > + resolve(); > > + }, {once: true}); > > + this._switcherPanel.hidePopup(); > > + }); > > I don't see us ever using the promise here? > > ::: browser/base/content/browser-sidebar.js:87 > (Diff revision 12) > > + this._switcherPanel.addEventListener("popupshown", () => { > > + resolve(); > > + }, {once: true}); > > If we hide the popup while showing it (before popupshown fires), this never > resolves, I think? I also think that this never gets used except in tests, > so I wonder if we should just add the promise stuff as a helper in the test > and not bother with it here... OK, will remove the Promise stuff. > ::: browser/base/content/browser.xul:1091 > (Diff revision 12) > > <deck id="content-deck" flex="1"> > > <hbox flex="1" id="browser"> > > <vbox id="browser-border-start" hidden="true" layer="true"/> > > <vbox id="sidebar-box" hidden="true" class="chromeclass-extrachrome"> > > <sidebarheader id="sidebar-header" align="center"> > > - <label id="sidebar-title" persist="value" flex="1" crop="end" control="sidebar"/> > > + <panel id="sidebarMenu-popup" > > My experience is that adding popups as children of other nodes leads to > pain. Specifically pain like in bug 1227540. In this particular case, I > don't see a lot of upsides - I don't think we need to keep the panel here > because of styling. > > Can you add it to #mainPopupSet instead? I think this may require one selector to change: `#sidebar-box #sidebarMenu-popup:not([hidden]) ~ #sidebar-switcher-target`. But we could also add a class to the switcher target on popupshowing and remove it on popuphidden, and I believe this would be equivalent. > ::: browser/base/content/browser.xul:1098 > (Diff revision 12) > > + role="group" > > + type="arrow" > > + hidden="true" > > + flip="slide" > > + position="bottomcenter topleft" > > + noautofocus="true" > > Do we need the noautofocus? See above > ::: browser/base/content/browser.xul:1100 > (Diff revision 12) > > + <panelmultiview id="sidebarMenu-multiView" mainViewId="sidebarMenu-mainView"> > > + <panelview id="sidebarMenu-mainView" class="cui-widget-panelview PanelUI-subView"> > > + <vbox class="panel-subview-body"> > > I don't believe we're planning to ever show subviews in here, so I think you > can omit the <panelmultiview> and <panelview> elements. Unless that makes > the styling story a lot more difficult? I'll go for that and see what we need to do to keep the styling. > ::: browser/base/content/browser.xul:1133 > (Diff revision 12) > > + oncommand="SidebarUI.hide()"/> > > + </vbox> > > + </panelview> > > + </panelmultiview> > > + </panel> > > + <toolbarbutton id="sidebar-switcher-target" flex="1" class="tabbable"> > > On Windows, after clicking an item, I see focus rings on the button. What's > causing this? There shouldn't be focus rings unless I start using [Tab] to > move focus around. Not sure, I can take a look > ::: browser/base/content/browser.xul:1139 > (Diff revision 12) > > + <image id="sidebar-icon"/> > > + <label id="sidebar-title" persist="value" crop="end" control="sidebar"/> > > + <image id="sidebar-switcher-arrow"/> > > + <spacer flex="1"/> > > + </toolbarbutton> > > <image id="sidebar-throbber"/> > > You seem to have left the markup but, AFAICT, are removing all the styling. > Should we remove the markup as well? Why are we removing the styling? > There's now no more feedback about pages that load slowly, which is a bit > sad. :-( Didn't intend to remove the styling, will re-add. > ::: browser/base/content/test/sidebar/browser.ini:3 > (Diff revision 12) > > +[DEFAULT] > > + > > +[browser_sidebar_switcher.js] > > Can you also move browser_bug409481.js while we're here? Sure, will do. > ::: browser/base/content/test/sidebar/browser_sidebar_switcher.js:5 > (Diff revision 12) > > + > > +add_task(function* () { > > + // If a sidebar is already open, close it. > > + if (!document.getElementById("sidebar-box").hidden) { > > + info("Unexpected sidebar found - a previous test failed to cleanup correctly"); > > Should this ok(false, ...) ? Fine with me.. I copied this from https://dxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_sidebarpanels_click.js?q=%22Unexpected+sidebar+found%22&redirect_type=single#20, so I'll update that as well. > ::: browser/base/content/test/sidebar/browser_sidebar_switcher.js:15 > (Diff revision 12) > > + switcherPromise = Promise.all([ > > + BrowserTestUtils.waitForEvent(window, "SidebarFocused"), > > + BrowserTestUtils.waitForEvent(sidebarPopup, "popuphidden"), > > + ]); > > 3 repetitions... should we stick this in a helper function? :-) > > ::: browser/themes/shared/sidebar/bookmark-16-outline.svg:1 > (Diff revision 12) > > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > > I guess more generally... it's a bit unfortunate that because we're all > working on different bits (across photon, not just structure), it's hard to > see how to best package things such that paths are relatively consistent and > we reuse identical icons. All of these icons look like they're essentially > the same icons we'll be using for toolbar buttons and/or menu panel items. > So I'm not sure sidebar/ is the best place for them. Equally, I'm not sure > if it's premature to create a single icons/ folder or something like that > (plus that then quickly becomes a dumping ground). > > Let's leave it like this for now, and if we find we need/want to reuse > things, I guess we can unify things then. Sounds good to me

Aaron, here are three points from the review that would be great to have some feedback on: > ::: browser/themes/shared/sidebar.inc.css:42 > (Diff revision 12) > > +#sidebar-title { > > + margin: 0; > > + padding: 0; > > + padding-inline-start: 8px; > > + padding-inline-end: 4px; > > + font-size: var(--title-font-size); > > We don't normally set font size so that we work correctly when people modify > default Windows font sizes. If we do really need to set a font-size, setting > it in em would be preferable, I think. This was based on the spec at https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/231191185_Explainer_-_Sidebar. Should we set the font in px, em, or not at all? > ::: browser/themes/shared/sidebar.inc.css:56 > (Diff revision 12) > > +} > > + > > +#sidebar-close { > > + -moz-appearance: none; > > + -moz-context-properties: fill; > > + fill: var(--icon-fill); > > This is hardcoded so it doesn't work in the dark compact theme (on Windows, > at least). Same for the icon displayed in the header and the down arrow. Did > currentColor, as Dão suggested, not work? I got this color from shorlander on slack. I'm happy to update to currentColor. It does make the icons darker with the default theme but getting it to work with dark theme is probably worth it. > Nit: can you add a % commented out license header? > ::: browser/themes/shared/sidebar/bookmark-16-outline.svg:1 > (Diff revision 12) > > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > > For all of these, can we drop the -16- infix, add a license header, and drop > the viewBox attribute? I can, but really all the icons in Drive should be updated to match this since this will be a problem for every new feature using the icon.

Comment on attachment 8862170 [details] Bug 1355324 - Create a popup to switch between sidebars from the sidebar header; https://reviewboard.mozilla.org/r/134156/#review141778 ::: browser/themes/shared/sidebar.inc.css:35 (Diff revision 12) > + border-inline-start-width: 1px; > + margin-inline-start: 0; > + margin-inline-end: -3px; > +} > + > +/* TODO: Why both by ID and class? */ Gijs, do you know why both .sidebar-title and #sidebar-title were specified (same with .sidebar-throbber and #sidebar-throbber)?

ni? for Comment 35

(In reply to :Gijs from comment #32) > ::: browser/base/content/browser.xul:1133 > (Diff revision 12) > > + oncommand="SidebarUI.hide()"/> > > + </vbox> > > + </panelview> > > + </panelmultiview> > > + </panel> > > + <toolbarbutton id="sidebar-switcher-target" flex="1" class="tabbable"> > > On Windows, after clicking an item, I see focus rings on the button. What's > causing this? There shouldn't be focus rings unless I start using [Tab] to > move focus around. I think it must be the tabbable class which just sets -moz-user-focus: normal. I see the same thing by the way if I mousedown on the close button in the sidebar but then move the mouse off of it before completing the click. Is this not the default behavior for focusable elements in Windows? How do tabs accomplish the "only show ring when receiving keyboard focus" behavior?

This patch includes all the feedback except for: 1) the font size / color (waiting on ux response) 2) the windows focus thing (see Comment 37) 3) the noautofocus/consumeoutsideclicks which appears to still be needed to prevent re-opening the popup when clicking on the anchor in linux. Maybe I'm missing another option (I found these attributes through trial and error), but when the click happens the panel is already closed so toggleSwitcherPanel just reshows it

(In reply to Brian Grinstead [:bgrins] from comment #35) > Comment on attachment 8862170 [details] > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > header; > > https://reviewboard.mozilla.org/r/134156/#review141778 > > ::: browser/themes/shared/sidebar.inc.css:35 > (Diff revision 12) > > + border-inline-start-width: 1px; > > + margin-inline-start: 0; > > + margin-inline-end: -3px; > > +} > > + > > +/* TODO: Why both by ID and class? */ > > Gijs, do you know why both .sidebar-title and #sidebar-title were specified > (same with .sidebar-throbber and #sidebar-throbber)? Not off-hand. Blame suggests this was a result of Social API ( https://hg.mozilla.org/mozilla-central/rev/d3ef31b50bf9 ). Not sure if this is dead already and the classes could just be removed. Shane or Mark should know.

(In reply to Brian Grinstead [:bgrins] from comment #39) > 3) the noautofocus/consumeoutsideclicks which appears to still be needed to > prevent re-opening the popup when clicking on the anchor in linux. Maybe > I'm missing another option (I found these attributes through trial and > error), but when the click happens the panel is already closed so > toggleSwitcherPanel just reshows it There's a "consumeanchor" attribute for what the anchor for consuming clicks should be, if it's bigger than the anchor we use (like, if we anchor on an icon in a button, and we should consume clicks on the entire button). See bug 987230. Maybe that helps here?

I should get to a final review by Monday, sorry for the delay (and bugspam) - I'm just getting back home and still jetlagged. :-)

(In reply to :Gijs from comment #40) > (In reply to Brian Grinstead [:bgrins] from comment #35) > > Comment on attachment 8862170 [details] > > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > > header; > > > > https://reviewboard.mozilla.org/r/134156/#review141778 > > > > ::: browser/themes/shared/sidebar.inc.css:35 > > (Diff revision 12) > > > + border-inline-start-width: 1px; > > > + margin-inline-start: 0; > > > + margin-inline-end: -3px; > > > +} > > > + > > > +/* TODO: Why both by ID and class? */ > > > > Gijs, do you know why both .sidebar-title and #sidebar-title were specified > > (same with .sidebar-throbber and #sidebar-throbber)? The socialapi sidebar was a separate (right side) sidebar that duplicated the look/style of the left-side sidebar. However, it couldn't reuse the id's used in the left side, so we added classes that were needed. I'm guessing the classes are no longer used since socialapi sidebar was removed (and probably just missed those items in removal).

(In reply to Shane Caraveo (:mixedpuppy) from comment #43) > (In reply to :Gijs from comment #40) > > (In reply to Brian Grinstead [:bgrins] from comment #35) > > > Comment on attachment 8862170 [details] > > > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > > > header; > > > > > > https://reviewboard.mozilla.org/r/134156/#review141778 > > > > > > ::: browser/themes/shared/sidebar.inc.css:35 > > > (Diff revision 12) > > > > + border-inline-start-width: 1px; > > > > + margin-inline-start: 0; > > > > + margin-inline-end: -3px; > > > > +} > > > > + > > > > +/* TODO: Why both by ID and class? */ > > > > > > Gijs, do you know why both .sidebar-title and #sidebar-title were specified > > > (same with .sidebar-throbber and #sidebar-throbber)? > > The socialapi sidebar was a separate (right side) sidebar that duplicated > the look/style of the left-side sidebar. However, it couldn't reuse the > id's used in the left side, so we added classes that were needed. I'm > guessing the classes are no longer used since socialapi sidebar was removed > (and probably just missed those items in removal). OK, thanks. I hadn't seen any references to the class names in m-c so that explanation makes sense. For sidebar-throbber I don't even see any in addons search, but for sidebar-title there are around 80 results for class="sidebar-title": https://dxr.mozilla.org/addons/search?q=class%3Dsidebar-title%22&redirect=false. I could remove the sidebar-throbber class but keep the sidebar-title for now..

(In reply to :Gijs from comment #41) > (In reply to Brian Grinstead [:bgrins] from comment #39) > > 3) the noautofocus/consumeoutsideclicks which appears to still be needed to > > prevent re-opening the popup when clicking on the anchor in linux. Maybe > > I'm missing another option (I found these attributes through trial and > > error), but when the click happens the panel is already closed so > > toggleSwitcherPanel just reshows it > > There's a "consumeanchor" attribute for what the anchor for consuming clicks > should be, if it's bigger than the anchor we use (like, if we anchor on an > icon in a button, and we should consume clicks on the entire button). See > bug 987230. Maybe that helps here? Ah interesting - I didn't realize when previously testing that the behavior was OK when clicking on the anchor. Adding a consumeanchor on the sidebar-icon totally seems to work, thanks!

(In reply to Brian Grinstead [:bgrins] from comment #37) > (In reply to :Gijs from comment #32) > > ::: browser/base/content/browser.xul:1133 > > (Diff revision 12) > > > + oncommand="SidebarUI.hide()"/> > > > + </vbox> > > > + </panelview> > > > + </panelmultiview> > > > + </panel> > > > + <toolbarbutton id="sidebar-switcher-target" flex="1" class="tabbable"> > > > > On Windows, after clicking an item, I see focus rings on the button. What's > > causing this? There shouldn't be focus rings unless I start using [Tab] to > > move focus around. > > I think it must be the tabbable class which just sets -moz-user-focus: > normal. I see the same thing by the way if I mousedown on the close button > in the sidebar but then move the mouse off of it before completing the > click. Is this not the default behavior for focusable elements in Windows? > How do tabs accomplish the "only show ring when receiving keyboard focus" > behavior? Just an update after some more testing. At least on Win7 if I've tabbed into anything first (like from url bar to search bar), then clicking on the switcher target and/or sidebar close button shows the focus ring. If you just open the browser then click onto either target, the focusring doesn't show up. Which is the same behavior as the identity-box. AFAICT tab labels are the outlier.

Comment on attachment 8862170 [details] Bug 1355324 - Create a popup to switch between sidebars from the sidebar header; https://reviewboard.mozilla.org/r/134156/#review142528 I think this basically looks good besides the few points raised below and the ones we're waiting on Aaron for. :-) ::: browser/base/content/browser.xul:293 (Diff revision 14) > </arrowscrollbox> > </hbox> > <hbox id="share-container" flex="1"/> > </panel> > + <panel id="sidebarMenu-popup" > + class="cui-widget-panel" We landed a bunch of styling changes, it'd be worth checking if this is necessary/sufficient for making the panel look OK against current tip. :-) ::: browser/base/content/browser.xul:299 (Diff revision 14) > + noautofocus="true" > + consumeoutsideclicks="true"> You've added the consumeanchor thing, do we still need these? ::: browser/base/content/browser.xul:301 (Diff revision 14) > + hidden="true" > + flip="slide" > + position="bottomcenter topleft" > + noautofocus="true" > + consumeoutsideclicks="true"> > + <vbox> Super nitty nit: This vbox is 2-space indented but the buttons in it are 4-space indented. :-) Perhaps more important: do we really need the vbox? Perhaps you can just set orient=vertical on the panel? Not sure. :-)

(In reply to :Gijs from comment #48) > Comment on attachment 8862170 [details] > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > header; > > https://reviewboard.mozilla.org/r/134156/#review142528 > > I think this basically looks good besides the few points raised below and > the ones we're waiting on Aaron for. :-) > > ::: browser/base/content/browser.xul:293 > (Diff revision 14) > > </arrowscrollbox> > > </hbox> > > <hbox id="share-container" flex="1"/> > > </panel> > > + <panel id="sidebarMenu-popup" > > + class="cui-widget-panel" > > We landed a bunch of styling changes, it'd be worth checking if this is > necessary/sufficient for making the panel look OK against current tip. :-) Just tested, and it's definitely still needed. But I'm not sure if it's sufficient, it looks like some styling has been attached to the photonpanelmultiview eleemnt, however I can't tell if we want to use the same styling in the sidebar popup based on https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/231191185_Explainer_-_Sidebar > ::: browser/base/content/browser.xul:299 > (Diff revision 14) > > + noautofocus="true" > > + consumeoutsideclicks="true"> > > You've added the consumeanchor thing, do we still need these? No, not sure how those got back in the patch > ::: browser/base/content/browser.xul:301 > (Diff revision 14) > > + hidden="true" > > + flip="slide" > > + position="bottomcenter topleft" > > + noautofocus="true" > > + consumeoutsideclicks="true"> > > + <vbox> > > Super nitty nit: This vbox is 2-space indented but the buttons in it are > 4-space indented. :-) > > Perhaps more important: do we really need the vbox? Perhaps you can just set > orient=vertical on the panel? Not sure. :-) orient=vertical seems to work

Gijs, there’s a css rule for the separator that we are using for the main hamburger popup: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1279. I didn’t realize this at the time but when removing the subview elements we lost the styling for the separator between the sidebar panels and the close option. What’s your preference for getting the styling back: 1) Add in a selector that matches our panel to that list (pro=no duplicated css, con=putting styles unrelated to customizable ui in this file) 2) Copy/paste the styles into sidebar.inc.css with a single selector for separators in the sidebar And I guess more generally, as in the photonpanelmultiview part in Comment 49, how much do we want to share styles between the main panel ui and the sidebar panel ui. I'm assuming there are other panels also that need this kind of styling (although maybe they will all end up being multiviews so can reuse the main panel styling.

(In reply to Brian Grinstead [:bgrins] from comment #50) > Gijs, there’s a css rule for the separator that we are using for the main > hamburger popup: > https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > customizableui/panelUI.inc.css#1279. I didn’t realize this at the time but > when removing the subview elements we lost the styling for the separator > between the sidebar panels and the close option. What’s your preference for > getting the styling back: > > 1) Add in a selector that matches our panel to that list (pro=no duplicated > css, con=putting styles unrelated to customizable ui in this file) > 2) Copy/paste the styles into sidebar.inc.css with a single selector for > separators in the sidebar > > And I guess more generally, as in the photonpanelmultiview part in Comment > 49, how much do we want to share styles between the main panel ui and the > sidebar panel ui. I'm assuming there are other panels also that need this > kind of styling (although maybe they will all end up being multiviews so can > reuse the main panel styling. I think we're going to unify the styling before we ship, but we're trying not to rock the boat too much for existing stuff right now. Can we add .cui-widget-panelview toolbarseparator to that selector ? I would expect that to work and not feel too out of place, and make it easier to unify the styling later without breaking stuff. Sound OK?

(In reply to :Gijs from comment #51) > Can we add .cui-widget-panelview toolbarseparator to that selector ? I would > expect that to work and not feel too out of place, and make it easier to > unify the styling later without breaking stuff. Sound OK? Yes, but the selector will have to be `.cui-widget-panel toolbarseparator`. I'll update the patch

Comment on attachment 8862170 [details] Bug 1355324 - Create a popup to switch between sidebars from the sidebar header; https://reviewboard.mozilla.org/r/134156/#review142784 ::: browser/themes/shared/customizableui/panelUI.inc.css:1115 (Diff revision 15) > .PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .menu-text, > .PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .menu-iconic-text { > font: menu; > } > > -.PanelUI-subView .subviewbutton[shortcut]::after { > +.subviewbutton[shortcut]::after { I had to make this change to get keyboard shortcuts to show back up. It seemed safe to be looking for usages of the shortcut attribute, let me know if you'd rather have me add a new selector to the list instead of modifying this one.

ni? for Comment 54

(In reply to Brian Grinstead [:bgrins] from comment #54) > Comment on attachment 8862170 [details] > Bug 1355324 - Create a popup to switch between sidebars from the sidebar > header; > > https://reviewboard.mozilla.org/r/134156/#review142784 > > ::: browser/themes/shared/customizableui/panelUI.inc.css:1115 > (Diff revision 15) > > .PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .menu-text, > > .PanelUI-subView .subviewbutton:not(.panel-subview-footer) > .menu-iconic-text { > > font: menu; > > } > > > > -.PanelUI-subView .subviewbutton[shortcut]::after { > > +.subviewbutton[shortcut]::after { > > I had to make this change to get keyboard shortcuts to show back up. It > seemed safe to be looking for usages of the shortcut attribute, let me know > if you'd rather have me add a new selector to the list instead of modifying > this one. Looks OK to me, though note that there's a margin rule further down that you may want to adapt, too, in that case.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e589d6b01b8d Create a popup to switch between sidebars from the sidebar header;r=Gijs

¡Hola Brian! Thank you for this! Found a problem though, when using the dark theme the header is almost impossible to read (please see sidebar-heading.png attached). Shall a follow up bug be filed? ¡Gracias! Alex

(In reply to alex_mayorga from comment #60) > Created attachment 8868634 [details] > Heading hard to read when using dark compact theme > > ¡Hola Brian! > > Thank you for this! > > Found a problem though, when using the dark theme the header is almost > impossible to read (please see sidebar-heading.png attached). > > Shall a follow up bug be filed? Yes, please do file a follow up bug blocking this one

Verified on Windows, Mac, and Ubuntu.

Considering bug 1365705 and bug 1365807, I think this could use another QA round. Has this been tested with high contrast mode? What about keyboard access? E.g. there seems to be no focus indicator over here on Linux.

Understood. It seems there is a decision pending on how to go about that. I'll hold off on verification until we get a final answer on this and follow-up tomorrow in the daily call.

Managed to reproduce the issue on an affected build (Firefox 55.0b4, Build ID: 20170622104007), on Windows 10x64. [TestDay - 20170623]

Thanks for looking into this Fahima, but there are some bugs that relate to this one and we'll need to check functionality on Mac and Linux as well.

Tested with a dark sidebar theme and everything seems to look fine. Marking as Verified.