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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: alice0775, Assigned: mikedeboer)

Tracking

(Depends on 1 bug, Blocks 1 bug)

56 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 unaffected, firefox56 wontfix, firefox57 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
(Reporter)

Updated

2 years ago
Blocks: 1352110
Whiteboard: [photon-structure] → [photon-structure] [triage]

Updated

2 years ago
Assignee: nobody → bbell
Whiteboard: [photon-structure] [triage]
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.

Updated

2 years ago
Duplicate of this bug: 1389610

Updated

2 years ago
Whiteboard: [photon-structure] [triage]

Comment 3

2 years ago
Posted image Mockup.png (obsolete) —
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

2 years ago
Posted image Mockup.png
Invision Mock can be seen here: https://mozilla.invisionapp.com/share/UXD0H9B8H#/248374032_Library_-_Bookmarking_Tools
Attachment #8896548 - Attachment is obsolete: true

Comment 5

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

Updated

2 years ago
Assignee: bbell → mdeboer
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure] [triage] → [photon-structure]

Comment 7

2 years ago
mozreview-review
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.
Attachment #8897037 - Flags: review?(gijskruitbosch+bugs)
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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!
Attachment #8897037 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

2 years ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70729ad46076
Add a tools section and subview to the Bookmarks subview inside the Library widget. r=Gijs
(Assignee)

Updated

2 years ago
Summary: Hamburger menu > Library > Bookmarks makes me confuse, because there are no Bookmarks Menu/Bookmarks Toolbar/Other Bookmarks entry → Hamburger menu > Library > Bookmarks is confusing, because there are no Bookmarks Menu/Bookmarks Toolbar/Other Bookmarks entry

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70729ad46076
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly

Comment 13

2 years ago
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Too late for 56. Mass won't fix for 56.

Updated

2 years ago
Depends on: 1404263

Updated

7 months ago
Depends on: 1495433
You need to log in before you can comment on or make changes to this bug.