880399 - Move star button out from URL bar into the bookmarks-menu-button.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

Because the Firefox menu is gone, the Bookmarks menu is not immediately accessible (unless the user exposes the toolbar-menubar).

One access point to this is the bookmarks-menu-button, but this button is not always available on a user's nav-bar (sometimes it's been customized away).

Another access point is via the Bookmarks Toolbar.

I think UX wants to try taking the star out of the URL bar again - essentially, undoing bug 867343, so we at least can see how people feel about having the star outside of the URL bar.

It's not bug 855805, but it's a step in that direction.

Comment 1

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

Note that this should not hit the UX branch - we only want to prepare a patch for the user research build, which we'll generate via a try push.

Comment 2

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

Attachment: Reverse of backout patch

Here's a straight reverse of the backout patch. I'll de-bitrot this next.

Comment 3

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

Attachment: Patch v1

Just did the de-bitrot. Not yet tested.

Comment 4

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

Turns out UX'd be happy to have this land this land on UX (and maybe even slip into m-c) - though they might want to modify it a bit once it lands. Those modifications are still TBD, according to the last Australis checkpoint meeting.

Comment 5

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

Comment on attachment 759381 [details] [diff] [review]
Patch v1

This patch adds some bogus comment into nsBrowserGlue.js from when I thought this was only landing on our research build.

Comment 6

[Marco Bonardo [:mak]]

you're not renaming again BookmarkingUI to BookmarksMenuButton, right? :)

Comment 7

[Marco Bonardo [:mak]]

if you just want this for UR build, I'm not sure regarding the UI migration, adding such migration will completely break those profiles, is it not easier to not migrate anything for now and just ask them to create a new profile?

When it will land in central, then sure you can add migration, but it will differ from this one, version 12 should stay in place for example, it's just removing bookmarks-menu-button-container that is correct, your migration 15 should just add the button.

Comment 8

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

Attachment: Patch v1.1

This patch is for the UX branch, and depends on the patch in bug 880421.

Comment 9

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

Comment on attachment 759780 [details] [diff] [review]
Patch v1.1

Ok, I think this should get your star button working again, Marco. :)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 759780 [details] [diff] [review]
Patch v1.1

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

please correct the bug title to reality

I didn't apply the patch, a basic functionality manual testing before it lands would be appreciated.

::: browser/base/content/browser-places.js
@@ +1111,5 @@
>  
> +  _updateStyle: function BUI__updateStyle() {
> +    if (!this.star) {
> +      return;
> +    }

please keep the _updateToolbarStyle naming, it was an improvement after all.

The check for this.star should not be needed, just checking this.button is enough for this method

::: browser/base/content/browser.xul
@@ +875,5 @@
>        <toolbarbutton id="bookmarks-menu-button"
>                       class="toolbarbutton-1 chromeclass-toolbar-additional"
>                       persist="class"
>                       removable="true"
> +                     type="menu-button"

looks like you forgot to remove <image id="star-button"> from urlbar-icons?

::: browser/components/nsBrowserGlue.js
@@ +1421,5 @@
> +          }
> +          this._setPersist(toolbarResource, currentsetResource, currentset);
> +        }
> +      }
> +    }

As I previously said, bumping migrationUI in parallel to mozilla-central is very dangerous, I hope all UX users won't share a profile with central.

Comment 11

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

Attachment: Patch v1.2 (r+'d by mak)

(In reply to Marco Bonardo [:mak] from comment #10)
> Comment on attachment 759780 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 759780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please correct the bug title to reality
> 
> I didn't apply the patch, a basic functionality manual testing before it
> lands would be appreciated.
> 

Done, and it seems to work as advertised.

> ::: browser/base/content/browser-places.js
> @@ +1111,5 @@
> >  
> > +  _updateStyle: function BUI__updateStyle() {
> > +    if (!this.star) {
> > +      return;
> > +    }
> 
> please keep the _updateToolbarStyle naming, it was an improvement after all.
> 

Done.

> The check for this.star should not be needed, just checking this.button is
> enough for this method
> 

Removed the check for this.star.

> ::: browser/base/content/browser.xul
> @@ +875,5 @@
> >        <toolbarbutton id="bookmarks-menu-button"
> >                       class="toolbarbutton-1 chromeclass-toolbar-additional"
> >                       persist="class"
> >                       removable="true"
> > +                     type="menu-button"
> 
> looks like you forgot to remove <image id="star-button"> from urlbar-icons?
> 

Good catch! Removed.

> ::: browser/components/nsBrowserGlue.js
> @@ +1421,5 @@
> > +          }
> > +          this._setPersist(toolbarResource, currentsetResource, currentset);
> > +        }
> > +      }
> > +    }
> 
> As I previously said, bumping migrationUI in parallel to mozilla-central is
> very dangerous, I hope all UX users won't share a profile with central.

Hopefully our UX Nightly users know the risks. :) I'm not too worried, tbh.

Comment 12

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

Landed on UX as https://hg.mozilla.org/projects/ux/rev/fc79651935e7

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I tries this new bookmarks button on latest ux build.
I seem that the button showing a bookmarks menu is narrow as compared to before bookmarks menu button. I feel this needs more width for clicking it easy :(

Should I file a new bug to polish this feature after this bug will be landed?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #13)
> I tries this new bookmarks button on latest ux build.
> I seem that the button showing a bookmarks menu is narrow as compared to
> before bookmarks menu button. I feel this needs more width for clicking it
> easy :(
> 
> Should I file a new bug to polish this feature after this bug will be landed?

the widget is being redesigned in bug 855805, the new version has a larger and more visible dropdown, as large as the star button.

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #14)
> 
> the widget is being redesigned in bug 855805, the new version has a larger
> and more visible dropdown, as large as the star button.

Thank you. I follow bug 855805.

Comment 16

[:Gijs (he/him)]

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

Comment 17

[pal-moz]

user can no longer separate each other ?
eg. set star on the right-side of navbar, bookmark on the left-side.

Comment 18

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

(In reply to pal-moz from comment #17)
> user can no longer separate each other ?
> eg. set star on the right-side of navbar, bookmark on the left-side.

That's correct - we've bound the bookmark menu and star button to try to consolidate the actions of making bookmarks and finding / using bookmarks.

Comment 19

[BuTaMuH]

(In reply to Mike Conley (:mconley) from comment #18)
> (In reply to pal-moz from comment #17)
> > user can no longer separate each other ?
> > eg. set star on the right-side of navbar, bookmark on the left-side.
> 
> That's correct - we've bound the bookmark menu and star button to try to
> consolidate the actions of making bookmarks and finding / using bookmarks.

Wouldn't at least be possible to swap them, so that the star is on the right side ?

Comment 20

[Marco Bonardo [:mak]]

(In reply to BuTaMuH from comment #19)
> Wouldn't at least be possible to swap them, so that the star is on the right
> side ?

unlikely, that's a very subjective request and we can't unfortunately satisfy everyone in the core product.