1890680 - Sidebar launcher is obscured by the bookmarks toolbar

Status: RESOLVED FIXED

(Firefox :: Sidebar, defect, P2)

Description

[Sam Foster [:sfoster] (he/him)]

Attachment: sidebar-launcher-bookmarks-toolbar.png

When the bookmarks toolbar is visible, the sidebar launcher is overlapped by it, making it difficult to see and click the top-most icon.

Comment 1

[Sarah Clements [:sclements]]

This didn't end up getting ported over from the prototype - it should be a simple fix.

Comment 2

[Sarah Clements [:sclements]]

The requirements for this is to have the sidebar (both collapsed and expanded state) be placed to the left of the bookmarks toolbar if the sidebar is positioned to the left (there's a pre-existing "sidebar.position_start" pref). Spec is here.

If it's positioned to the right, the sidebar should sit below the bookmarks toolbar, per the spec here.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Sarah Clements [:sclements] from comment #2)

The requirements for this is to have the sidebar (both collapsed and expanded state) be placed to the left of the bookmarks toolbar if the sidebar is positioned to the left (there's a pre-existing "sidebar.position_start" pref). Spec is here.

This seems like a nonstarter given our current toolbar setup. I'd suggest just fixing this bug, and spinning of the above vision to a separate bug that probably shouldn't be considered a blocker until we have a tangible, robust plan for implementing that (or does that already exist?).

Comment 4

[Sarah Clements [:sclements]]

(In reply to Dão Gottwald [:dao] from comment #3)

(In reply to Sarah Clements [:sclements] from comment #2)

The requirements for this is to have the sidebar (both collapsed and expanded state) be placed to the left of the bookmarks toolbar if the sidebar is positioned to the left (there's a pre-existing "sidebar.position_start" pref). Spec is here.

This seems like a nonstarter given our current toolbar setup.

Can you elaborate on this?

Comment 5

[Dão Gottwald [:dao]]

The toolbar is a child of a toolbox element, along with the navigation toolbar. It's not possible (in a clean way) to divide the toolbox so that only the bookmarks toolbar is next to the sidebar. We'd have to move the bookmarks toolbar outside of the toolbox, which I'm sure is possible but also not something we've done before, so it there could be a long tail of adjustments to make in toolbar customization code etc.. Then, for the case of the sidebar being on the right, it seems like we'd need also need to move the toolbar dynamically? That could also cause problems. And this all needs to work with the special bookmarks toolbar setup that I introduced in bug 1705215, so that we don't regress performance. Long story short, there are a couple of unknowns here, and it's possible they all turn out to be minor, but without a thorough investigation I wouldn't want to commit to implementing this at this stage.

Comment 6

[Sarah Clements [:sclements]]

(In reply to Dão Gottwald [:dao] from comment #5)

The toolbar is a child of a toolbox element, along with the navigation toolbar. It's not possible (in a clean way) to divide the toolbox so that only the bookmarks toolbar is next to the sidebar. We'd have to move the bookmarks toolbar outside of the toolbox, which I'm sure is possible but also not something we've done before, so it there could be a long tail of adjustments to make in toolbar customization code etc.. Then, for the case of the sidebar being on the right, it seems like we'd need also need to move the toolbar dynamically? That could also cause problems. And this all needs to work with the special bookmarks toolbar setup that I introduced in bug 1705215, so that we don't regress performance. Long story short, there are a couple of unknowns here, and it's possible they all turn out to be minor, but without a thorough investigation I wouldn't want to commit to implementing this at this stage.

Ok, thanks for flagging and sharing this. We'll put implementation on hold for now - I've filed bug 1893214 and cc'd you on it so that whomever picks up this bug to investigate it can share their findings and discuss any approaches as needed.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:sclements, could you have a look please?

For more information, please visit BugBot documentation.

Comment 8

[Sarah Clements [:sclements]]

Something worth flagging that Gijs mentioned to me is we also need to think about notification bars overlapping the sidebar. So these in the top of the content area, one such notification is the one you see about restoring a session (for a new profile).

Comment 9

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #8)

Something worth flagging that Gijs mentioned to me is we also need to think about notification bars overlapping the sidebar. So these in the top of the content area, one such notification is the one you see about restoring a session (for a new profile).

This has the same problem as the issue with showing the sidebar next to the bookmarks toolbar: the notification box is a child of toolbox#navigator-toolbox which spans the full window width and sits above the hbox#browser element which contains the sidebar. So, if you trigger a notification box e.g. by BrowserSearch.removalOfSearchEngineNotificationBox("Google", "Foogle"), you'll see that box pushed down the #browser element and everything in it - which includes the sidebar. I'd suggest if we're considering changing this behavior we create a bug for it so we can triage and prioritize separately.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1890680 - ensure the only-on-newtab bookmarks toolbar doesnt overlap the sidebar. r=#sidebar-reviewers!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d43232eb7fd4
ensure the only-on-newtab bookmarks toolbar doesnt overlap the sidebar. r=sidebar-reviewers,desktop-theme-reviewers,dao,nsharpley

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d43232eb7fd4