1003364 - The toolbar context menu items (collapse/uncollapse toolbars, and enter customize mode) should be present on the context menu for the Bookmarks Toolbar Items

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Comment 1

[u428464]

This would make the context menu for the bookmarks toolbar very cluttered. There are already 10 items in the menu when right-clicking an empty space.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Guillaume C. [:ge3k0s] from comment #1)
> This would make the context menu for the bookmarks toolbar very cluttered.
> There are already 10 items in the menu when right-clicking an empty space.

Some of those items make no sense to be there for the empty-area case. This is indeed another thing that we should fix, but not part of this bug.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> (In reply to Guillaume C. [:ge3k0s] from comment #1)
> > This would make the context menu for the bookmarks toolbar very cluttered.
> > There are already 10 items in the menu when right-clicking an empty space.
> 
> Some of those items make no sense to be there for the empty-area case. This
> is indeed another thing that we should fix, but not part of this bug.

Filed bug 1006989 to fix this issue.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8418550 [details] [diff] [review]
Patch

I'm going to adjust the approach on this patch to use more of the preexisting _shouldShowMenuItem code.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8419193 [details] [diff] [review]
Patch v2

Review of attachment 8419193 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for everything except what's going on inside that Places controller. I don't 100% get it, and you'd probably be better to get a review from mak on it.

Comment 8

[Marco Bonardo [:mak]]

I think these should appear only in the empty area of the toolbar or on the chevron menu, in the same way we don't show these customize options on the urlbar, on the back button, on the searchbar or on other complex elements that have their own contextual options.
I agree with comment 1 that adding these to any toolbar contextual menu would make them extremely cluttered, plus break muscle memory and make Properties an harder target.
Off-hand looks like the patch is just adding this to any element on the bookmarks toolbar, am I wrong?

Comment 9

[Marco Bonardo [:mak]]

the other problem is that the controller is shared across all views, included the Library and the sidebar, I suspect (but didn't try) this may not be properly filtering out those views.
Btw, mano may have better ideas on how to "merge" separate commands into the same context, maybe you may try to ping him on IRC.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Marco, can you please add this to the backlog for this iteration as it is carry-over from before we started the new process but is close to completion.

Comment 12

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3.  Please provide a point estimates.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8419193 [details] [diff] [review]
Patch v2

Review of attachment 8419193 [details] [diff] [review]:
-----------------------------------------------------------------

I think comment 8 and comment 9 are blockers here, I think the discussion should start from the problems I pointed out.

Comment 14

[Marco Mucci [:MarcoM]]

Hi Jared, can you recommend if this bug should be [qa+] or [qa-] for QA verification.

Comment 15

[Marco Mucci [:MarcoM]]

Removed from Iteration 32.3

Comment 16

[Mike Hoye [:mhoye]]

Flagging as diamond; is this a good candidate for a mentored bug?

Comment 17

[:Gijs (he/him)]

(In reply to Mike Hoye [:mhoye] from comment #16)
> Flagging as diamond; is this a good candidate for a mentored bug?

It depends on bug 1006989, until that's fixed, I don't think this can reasonably be worked on by anyone. I hope to get to that in this iteration, and once that's landed I can update this bug appropriately.

Comment 18

[marvin]

Can somebody take another look at this? The context menu on empty space in the bookmark toolbar is quite short now. So adding the checkbox to hide the bookmarks toolbar there should be possible.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1003364 - Add Bookmarks Toolbar view options to the context menu of the Bookmarks Toolbar.

This patch doesn't show the view options when right-clicking on folders or bookmarks within the toolbar, and also doesn't include an item to enter Customize mode to keep the menu from growing too large.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfe8bc960f45
Add Bookmarks Toolbar view options to the context menu of the Bookmarks Toolbar. r=mak,Gijs

Comment 22

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_bookmarks_toolbar_context_menu_view_options.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/78788ef5e67d4307faa753500d789295fd572f89
Log link: https://treeherder.mozilla.org/logviewer?job_id=320867653&repo=autoland&lineNumber=8398

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d95d5167924
Add Bookmarks Toolbar view options to the context menu of the Bookmarks Toolbar. r=mak,Gijs

Comment 24

[Andreea Pavel [:apavel]]

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

Comment 25

[Daniel Cicas [:dcicas], Release QA]

Hello,

This issue is verified on Fx 84.0b4.