1389625 - Bookmark Menu object on the customize screen needs to be renamed "Bookmarks Menu"

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[bbell]

This name change is necessary because The Bookmark Menu is referenced as the "Bookmark Menu" in the Library where we offer users the option to "Open Bookmark Menu," and as a destination users can save bookmarks to in the save dialog where users can "Save to Bookmark Menu." However, you can't find a "Bookmark Menu" under customize, because it's labeled "Bookmarks."

Comment 1

[bbell]

Attachment: detail.png

Screenshot of what needs to be renamed.

Comment 2

[bbell]

(In reply to bbell from comment #0)
> This name change is necessary because The Bookmark Menu is referenced as the
> "Bookmark Menu" in the Library where we offer users the option to "Open
> Bookmark Menu," and as a destination users can save bookmarks to in the save
> dialog where users can "Save to Bookmark Menu." However, you can't find a
> "Bookmark Menu" under customize, because it's labeled "Bookmarks."

"Bookmarks Menu" (plural) should be the label.

Comment 3

[:Gijs (out for now; he/him)]

Attachment: Bug 1389625 - change bookmarks menu button label,

Review commit: https://reviewboard.mozilla.org/r/172154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172154/

Comment 4

[:Gijs (out for now; he/him)]

Attachment: Bug 1389625 - clean up subview headers now that we use the title of the view instead,

The panel-subview-header is always hidden in photon (sub)panels, and so we now never show it.
Removing it avoids having to readd the old label for the bookmarks view, remove some unused
strings, and I noticed that we accidentally left the PanelUI-sidebar container which is
unused since bug 1360282.

Review commit: https://reviewboard.mozilla.org/r/172156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172156/

Comment 5

[:Gijs (out for now; he/him)]

Taking this because of the strings involved.

One question I have here: when you put this button in the overflow panel (either pin it or when it appears there because there's no space on the toolbar), it will show the same bookmarks view as the library does (same items/content, same title).

There is also normally only 1 label for the button that gets used both in customize mode and in the panel. The labeling situation right now is:

palette: "Bookmarks"
panel: "Bookmarks"
title of the subview in panel: "Bookmarks"
title of the subview in library panel: "Bookmarks"


The situation after the patches I've just attached is:

palette: "Bookmarks Menu"
panel: "Bookmarks Menu"
title of the subview in panel: "Bookmarks"
title of the subview in library panel: "Bookmarks"


This is a bit confusing because you'd expect the thing in the panel to show you the "bookmarks menu" and it doesn't (though it will if you move it to any of the toolbars)

In principle, I could write code to make the label for the customize mode palette and the panel different, so it changes label when you move it between these items, but that also seems confusing.

Really, I don't know how to weigh this inconsistency with the one from comment #0. I think we're a bit out of time to implement a separate bookmarks menu view for the bookmarks menu button to use when it's in the overflow panel... (ie that opens any folders as subviews or by opening the library, the latter being what we did pre-photon... I guess we could try to resurrect that code from hg history... but it sounds hairy.)

Bryan, what would you like to do? Take this patch, keep the status quo, or something else?

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8900707 [details]
Bug 1389625 - change bookmarks menu button label,

https://reviewboard.mozilla.org/r/172154/#review177676

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8900708 [details]
Bug 1389625 - clean up subview headers now that we use the title of the view instead,

https://reviewboard.mozilla.org/r/172156/#review177678

Comment 8

[:Gijs (out for now; he/him)]

Discussed with Bryan, we're gonna go ahead with the rename.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1707066b3257
change bookmarks menu button label, r=adw
https://hg.mozilla.org/integration/autoland/rev/266a47a36a77
clean up subview headers now that we use the title of the view instead, r=adw

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1707066b3257
https://hg.mozilla.org/mozilla-central/rev/266a47a36a77

Comment 11

[Azmina]

I have reproduced this bug with Nightly 57.0a1 (2017-08-11) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1

Build ID : 20170830100404
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0