1892012 - Add toolbar entry point for the sidebar as a default setting

Status: RESOLVED FIXED

(Firefox :: Sidebar, task, P1)

Description

[Sarah Clements [:sclements]]

Make the existing sidebar toolbar icon entry point for the sidebar part of the default configuration for all users (Its currently in the Customization Panel as its default position). The spec is here. Make this change behind the existing sidebar pref.

The requirements for this are:

• The existing sidebar toolbar button is displayed to the right of the toolbar "Refresh" button, and if that has been removed, it should be to the right of the "Go forward" icon .
• If a user has customized their toolbar and added other tools to the right of the "Refresh" button, the sidebar icon shifts the user-added tools to the right.
• If a user has a custom home page (in which case Home button is added automatically next to the Refresh button), the sidebar icon is added to the right of the Home button.

For previous two points, there are helpful screenshots for macOS vs Windows in this section of the sidebar PRD (there's a link to it from the spec). We'll need to ensure the support/styling for windows and macOS.

Comment 1

[Sarah Clements [:sclements]]

One thing to mention since this came up during an estimation review, is that currently sidebars don't show up in popup windows. Per Gijs who did a bit of digging "We hide pretty much all the buttons from the toolbar to keep things minimal (but kept the hamburger menu for access reasons to e.g. print). However, you can still try to use the keyboard shortcuts, and the View > Sidebar menu on macOS. The menu entries are not disabled, and indeed the checkmark in front of the items appears/disappears, but to no avail (ie the sidebar never becomes visible).

I guess in an ideal world we fix this so the sidebar menu is disabled and the shortcuts no-op, but it's probably not critical given the existing state."

Edit: Let's tackle this in bug 1895868

Comment 2

[Kelly Cochrane [:kcochrane]]

Attachment: Bug 1892012 - Add toolbar entry point for the sidebar as a default setting

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e87839796b7
Add toolbar entry point for the sidebar as a default setting r=sidebar-reviewers,Gijs,mstriemer

Comment 4

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2e87839796b7

Comment 5

[Henry Wilkes (they/them) [:henry-x]]

@kcochrane What is the intended behaviour for this set of lines? As far as I can tell shouldAdd will always be false in this case, which will mean it won't have any effect further down.

Comment 6

[Kelly Cochrane [:kcochrane]]

Hi, Henry! If that were the case, then the test I added would be failing, and you wouldn't be seeing the sidebar button by default in the nav bar once the sidebar.revamp pref has been flipped unless you'd chosen to manually remove it afterwards.

In the conditional you linked, we're looking for if widget._introducedByPref && Services.prefs.getBoolPref(widget._introducedByPref) where widget._introducedByPref is set to "sidebar.revamp" here. If that pref is found and set to true, then we set shouldSetPref and shouldAdd to true only in the case that we haven't already set prefId (which is defined here - widget.id being "sidebar-button") to true already.

Hope that makes sense.

Comment 7

[Henry Wilkes (they/them) [:henry-x]]

@kcochrane thanks for explaining. I misread this

          shouldSetPref = shouldAdd = !Services.prefs.getBoolPref(
            prefId,
            false
          );

as

          shouldSetPref = shouldAdd == !Services.prefs.getBoolPref(
            prefId,
            false
          );

I.e. the second "=" I read as "==", so I thought shouldAdd wasn't set. My bad!

Thanks for explaining because I might have to adjust this for Tor Browser.