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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M7][User Research Build+])

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
Summary: Re-land star button changes for (at least) User Research build → Re-land star button changes for User Research build
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7][User Research Build+][Don't land me in UX, yo]
Created attachment 759369 [details] [diff] [review]
Reverse of backout patch

Here's a straight reverse of the backout patch. I'll de-bitrot this next.
Created attachment 759381 [details] [diff] [review]
Patch v1

Just did the de-bitrot. Not yet tested.
Attachment #759369 - Attachment is obsolete: true
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.
Whiteboard: [Australis:M7][User Research Build+][Don't land me in UX, yo] → [Australis:M7][User Research Build+]
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.
Attachment #759381 - Attachment is obsolete: true
you're not renaming again BookmarkingUI to BookmarksMenuButton, right? :)
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.
Created attachment 759780 [details] [diff] [review]
Patch v1.1

This patch is for the UX branch, and depends on the patch in bug 880421.
Comment on attachment 759780 [details] [diff] [review]
Patch v1.1

Ok, I think this should get your star button working again, Marco. :)
Attachment #759780 - Flags: review?(mak77)

Updated

5 years ago
Blocks: 865676
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.
Attachment #759780 - Flags: review?(mak77) → review+
Summary: Re-land star button changes for User Research build → Re-land star button changes
Summary: Re-land star button changes → Move star button out from URL bar into the bookmarks-menu-button.
Created attachment 759857 [details] [diff] [review]
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.
Attachment #759780 - Attachment is obsolete: true
Attachment #759857 - Flags: review+
Landed on UX as https://hg.mozilla.org/projects/ux/rev/fc79651935e7
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7][User Research Build+][fixed-in-ux]
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?
(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.
(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

4 years ago
https://hg.mozilla.org/mozilla-central/rev/fc79651935e7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][User Research Build+][fixed-in-ux] → [Australis:M7][User Research Build+]
Target Milestone: --- → Firefox 28

Comment 17

4 years ago
user can no longer separate each other ?
eg. set star on the right-side of navbar, bookmark on the left-side.
(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.
Flags: needinfo?(mconley)

Comment 19

4 years ago
(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 ?
(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.
You need to log in before you can comment on or make changes to this bug.