1377968 - Hamburger menu > Library > Bookmarks is confusing, because there are no Bookmarks Menu/Bookmarks Toolbar/Other Bookmarks entry

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Alice0775 White]

Reproducible : always

Steps To Reproduce:
1. Open Hamburger menu > Library > Bookmarks

Actual Results:
There are no Bookmarks Menu/Bookmarks Toolbar/Other Bookmarks entry.
Recent bookmarked one?


Expected Results:
I expected that it should be ordinary bookmarks menu.
Bookmarks Toolbar
Other Bookmarks
and Bookmarks Menu entry


OR It should be renamed "Recent Bookmarked" instead of "Bookmarks"
(Hamburger menu > Library > Recent Bookmarked)

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Just adding here that this is effectively removing the bookmark menu functionality in Windows right now, since there is no permanently visible menu bar that the bookmark menu could be accessed from.

Comment 3

[bbell]

Attachment: Mockup.png

The fix is to add a "Bookmarking Tools" section to the Library>Bookmarks Area. Users looking for the Bookmarks Menu will quickly find a way to return it to their toolbar. This change can also help introduce users to the Bookmarks Toolbar, and the Bookmarks Sidebar. 

See Attached Mockup:

Comment 4

[bbell]

Attachment: Mockup.png

Invision Mock can be seen here: https://mozilla.invisionapp.com/share/UXD0H9B8H#/248374032_Library_-_Bookmarking_Tools

Comment 5

[bbell]

Attachment: Library-BookmarkingTools-AllOn.png

If all three items are turned on the menu would look like this.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1377968 - Add a tools section and subview to the Bookmarks subview inside the Library widget.

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

Comment 7

[:Gijs (he/him)]

Comment on attachment 8897037 [details]
Bug 1377968 - Add a tools section and subview to the Bookmarks subview inside the Library widget.

https://reviewboard.mozilla.org/r/168316/#review173546

Nice, almost there!

::: browser/base/content/browser-places.js:1939
(Diff revision 1)
> +    viewNode.addEventListener("ViewShowing", () => {
> +      for (let button of [...viewNode.getElementsByTagName("toolbarbutton")]) {

Sorry, I'm being dim - can you explain why we need to do this in a ViewShowing listener? Why can't we do it immediately?

::: browser/base/content/browser-places.js:1972
(Diff revision 1)
> +      // Button is in the palette, so we can move it to the navbar.
> +      let searchbarPlace = CustomizableUI.getPlacementOfWidget("search-container");
> +      if (searchbarPlace && searchbarPlace.area == area) {
> +        pos = searchbarPlace.position + 1;
> +      } else {
> +        pos = CustomizableUI.getPlacementOfWidget("urlbar-container").position + 1;

This adds it between the urlbar container and the flexible space, if any. That seems suboptimal - can you avoid doing that? :-)

::: browser/base/content/browser-sidebar.js:285
(Diff revision 1)
>     * specified commandID. Otherwise the sidebar will be hidden.
>     *
>     * @param {string} commandID ID of the xul:broadcaster element to use.
>     * @return {Promise}
>     */
> -  toggle(commandID = this.lastOpenedId) {
> +  toggle(commandID = this.lastOpenedId, triggerNode) {

Please update the jsdoc here and below.

::: browser/base/content/browser-sidebar.js:395
(Diff revision 1)
>    },
>  
>    /**
>     * Hide the sidebar.
>     */
> -  hide() {
> +  hide(triggerNode) {

And here.

::: browser/components/customizableui/content/panelUI.js:374
(Diff revision 1)
> +    if (typeof aViewId == "string") {
> +      viewNode = document.getElementById(aViewId);
> +    } else if (aViewId.id) {
> +      viewNode = aViewId;
> +      aViewId = viewNode.id
> +    }
>      if (!viewNode) {
>        Cu.reportError("Could not show panel subview with id: " + aViewId);
>        return;
>      }

Now that we have no more add-ons, can we just pass the view ID / view node directly to container.showSubView and not worry about whether it's a string or an ID? I think we can just move the viewNode line to the `else if` check for the `container` thing, and remove the `!viewNode` check.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8897037 [details]
Bug 1377968 - Add a tools section and subview to the Bookmarks subview inside the Library widget.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168316/diff/1-2/

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8897037 [details]
Bug 1377968 - Add a tools section and subview to the Bookmarks subview inside the Library widget.

https://reviewboard.mozilla.org/r/168316/#review173546

> Now that we have no more add-ons, can we just pass the view ID / view node directly to container.showSubView and not worry about whether it's a string or an ID? I think we can just move the viewNode line to the `else if` check for the `container` thing, and remove the `!viewNode` check.

We could, but I chose to not touch this code and pass the ID (string) after all, because it triggers the PanelMultiView reparenting logic, which we need and want here anyways. I can update _that_ logic to be smarter too, but that felt like scope-creep here.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8897037 [details]
Bug 1377968 - Add a tools section and subview to the Bookmarks subview inside the Library widget.

https://reviewboard.mozilla.org/r/168316/#review173990

Nice!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70729ad46076
Add a tools section and subview to the Bookmarks subview inside the Library widget. r=Gijs

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70729ad46076

Comment 13

[u554753]

Verified on Windows, Mac, and Ubuntu.

Comment 14

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.