867343 - Back out star button UI changes

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Bug 748894 extracted the star button out from the URL bar and put it into a split button.

Today, the UX team told me that after a work week, they've got another direction they'd like to take with the star button (being tracked in bug 855805).

Jared and I were discussing this with fang, limi and shorlander, and I think the idea is to back out bug 748894 just after it hits the Aurora channel, before the first nightly build.

Marco - sorry for doing this, especially after I put on the pressure to have bug 748894 landed so I could get bug 855805 going. But it sounds like the intermediate step that we currently have on Nightly is not something we want to ship.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

After bug 853071 lands, you could put this behind an #ifdef NIGHTLY_BUILD?

Comment 2

[Marco Bonardo [:mak]]

I'd like to get involved (or at least informed) in changes regarding my projects, so far I don't even know the reasons behind the choice, an e-mail thread would have been welcome.

Due to code refactorings this will make very hard porting related fixes to Aurora.
Also note the UI migration step is tricky and must be explicitly handled when the Aurora merge happens.
It will also make hard to properly track the dependencies, since those bugs are fixed in Nightly, and will have to be marked wontfix on each new version until the new solution is ready, to avoid confusion.

Since I don't know the ETA of the new solution, nor the reasoning, I can't make an informed point, though I can evaluate the current situation. What we have in Nightly is an actual improvement versus the old behavior, solving various annoyances bugs in the UI, I'm not sure the decision to back it out waiting for a future solution is the right one, unless the future solution will be in users hands in a couple months.

Comment 3

[Marco Bonardo [:mak]]

So, my question is: why we can't take an intermediate solution while waiting for Australis, that regardless will change basically everything?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Bug 855805 would be the future solution, which is targeted for Australis.

The short story here is that after this was implemented, there were quite a few issues brought up around the interaction of the new control. If we can ship bug 855805 6 weeks after, then changing the UI twice within 6 weeks would be undesirable, hence the idea to just postpone any visible UI change for 6 weeks.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Jared Wein [:jaws] from comment #4)
> The short story here is that after this was implemented, there were quite a
> few issues brought up around the interaction of the new control.

Where are those reported, provided you refer to the Nightly menu-button?

> If we can
> ship bug 855805 6 weeks after, then changing the UI twice within 6 weeks
> would be undesirable

That may be fine provided it's 6 weeks, I really hope it is, but it looks an optimistic guess to me. I'm definitely happy if we release Australis in 6 weeks, but that means it should be in Nightly in about 10 days. Is that feasible?

That said, as I expressed by mail, I think it's a non-issue, cause there are 2 changes:
1. move the star to a separate widget
2. change 1-click bookmarking behavior

Looks like there wouldn't be anything wrong in doing those at different times, a different story would be to move a ui piece to a separate widget and then move it back.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #5)
> > If we can
> > ship bug 855805 6 weeks after, then changing the UI twice within 6 weeks
> > would be undesirable
> 
> That may be fine provided it's 6 weeks, I really hope it is, but it looks an
> optimistic guess to me. I'm definitely happy if we release Australis in 6
> weeks, but that means it should be in Nightly in about 10 days. Is that
> feasible?

Australis is targeting 24, which doesn't require it being on Nightly in 10 days. Assuming it does make 24, it would ship to release users 6 weeks after the star button changes (which made 23).

I think you and Mike came to an agreement per email. Mike, can you re-summarize the current plan here?

Comment 7

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> (In reply to Marco Bonardo [:mak] from comment #5)
> > > If we can
> > > ship bug 855805 6 weeks after, then changing the UI twice within 6 weeks
> > > would be undesirable
> > 
> > That may be fine provided it's 6 weeks, I really hope it is, but it looks an
> > optimistic guess to me. I'm definitely happy if we release Australis in 6
> > weeks, but that means it should be in Nightly in about 10 days. Is that
> > feasible?
> 
> Australis is targeting 24, which doesn't require it being on Nightly in 10
> days. Assuming it does make 24, it would ship to release users 6 weeks after
> the star button changes (which made 23).
> 
> I think you and Mike came to an agreement per email. Mike, can you
> re-summarize the current plan here?

Yes, thanks for reminding me to mention it in this bug.

Our plan is to construct a partial back-out patch for 23 Nightly that reverts the surface changes of bug 748894, while keeping the architectural improvements. The patch will also introduce a migration that will revert the new bookmark star button back to the bookmark menu button for Nightly profiles that have gotten the star.

Comment 8

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: WIP Patch 1

I think I've reverted the surface behaviour. The themes still need to be updated for Linux and OSX, but Windows seems OK.

Marco - is there anything big I'm missing here? Anything I've overlooked, or that might be broken that I haven't noticed?

Comment 9

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: WIP 2

Cleaned up a few things in browser/base/content/browser.css. I think I'm going to finish a draft of the Linux and OSX theme changes, and *then* request feedback.

Comment 10

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: WIP Patch 3

Adds styling for OSX.

Comment 11

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1

Ok, that's all 3 OS's.

So what am I overlooking? And what have I forgotten to remove?

Comment 12

[Marco Bonardo [:mak]]

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

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

Things to verify, the button when moved to the tabbar should properly support inverted style on glass.
When dragged to the bookmarks toolbar it should become much more compact, should not increase size of the toolbar.
it should also retain the "library" icon in all of those positions while from the css here looks like it's becoming a star in the bookmarks toolbar.

I did not yet compile the patch in and tested it, but will surely do with next iteration.

::: browser/base/content/browser-places.js
@@ +999,2 @@
>  
> +let BookmarksStarButton = {

Hm, we are going to change this everytime we change the widget somehow, I wonder if we should have a more generic object here, keeping it a single object. The immediate advantage is that you can avoid moving code across 2 objects, that makes review harder and increases possibility of unexpected bugs. It's a bit hard to follow the patch now with all of the code moves.
We may call that BookmarkingUI maybe?

@@ +1038,3 @@
>      }
>      else {
>        this.star.removeAttribute("disabled");

fwiw disabling the star is pointless since it gets hidden by a css rule, but I think we can keep this.

::: browser/base/content/browser.css
@@ +330,4 @@
>  toolbar[mode="text"] #bookmarks-menu-button > .toolbarbutton-menubutton-button > .toolbarbutton-text,
>  toolbar[mode="full"] #bookmarks-menu-button.bookmark-item > .toolbarbutton-menubutton-button > .toolbarbutton-text {
>    display: none;
>  }

these should be removed and reverted, the menu makes sense as text, and moreover when moved to the bookmarks toolbar should have a text label, as before.

::: browser/base/content/browser.xul
@@ +631,5 @@
>                       ondragenter="PlacesMenuDNDHandler.onDragEnter(event);"
>                       ondragover="PlacesMenuDNDHandler.onDragOver(event);"
>                       ondragleave="PlacesMenuDNDHandler.onDragLeave(event);"
>                       ondrop="PlacesMenuDNDHandler.onDrop(event);"
>                       oncommand="BookmarksMenuButton.onCommand(event);">

The oncommand here should be removed since now it's handled by the star.

You should also add back Bookmark This Page to the popup, it was removed cause the button had the star already, so it was redundant (see BMB_bookmarkThisPage in the original patch)

The other problem here is that we are going to move bookmarks-menu-button from wherever it is, I don't think we can undo that for Nightly users who already went through the migration, though we may modify the nsBrowserGlue migration for the others, so that if it finds bookmarks-menu-button-container, it replaces it with bookmarks-menu-button, instead of placing it elsewhere.

::: browser/themes/linux/browser.css
@@ +1459,5 @@
> +}
> +
> +#star-button[starred="true"] {
> +  list-style-image: url("chrome://browser/skin/places/pageStarred.png");
> +}

bookmark-item rules are missing

::: browser/themes/osx/browser.css
@@ +1695,4 @@
>    }
>  
> +  #star-button:hover:active[starred="true"] {
> +    -moz-image-region: rect(0, 96px, 32px, 64px);

the bookmark-item rules are missing

::: browser/themes/windows/browser.css
@@ +1748,5 @@
>    -moz-image-region: rect(0px 16px 16px 0px);
>  }
>  
>  #bookmarks-menu-button.bookmark-item[starred] {
>    -moz-image-region: rect(0px 48px 16px 32px);

these are wrong, it should not have the star icon when moved to the bookmarks toolbar.
see the bookmark-item rules in the patch

Comment 13

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1.1

Thanks for the feedback, Marco. I think I addressed all of the concerns you brought up in your comment. Going to test this on Ubuntu and OSX, and then I'll request review.

Comment 14

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1.2

Some follow-ups for OSX and Linux.

Comment 15

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1.3

Comment 16

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1.4

Getting rid of some dumps that shouldn't have been there.

Comment 17

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Comment on attachment 747006 [details] [diff] [review]
Patch v1.4

Ok, I think I've taken care of the issues you brought up. I also tested the cases you mentioned at the start of your last feedback, and we seem to be OK on that front too.

This patch seems to cause browser_bug432599.js to fail (at least on Ubuntu), and I'm not sure why. It looks like the star is being clicked properly, and the panel is opening, but it's closing before the onPopupHidden event handler is attached. Any ideas what I've missed?

Comment 18

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Hrm. browser_bug432599.js seems to only fail on my Ubuntu machine. Windows and OSX are just fine. :/

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 747006 [details] [diff] [review]
Patch v1.4

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

The patch looks overall good, now I'm moving towards testing

::: browser/base/content/browser-places.js
@@ +1106,3 @@
>    },
>  
> +  _updateStyle: function BUI__updateStyle() {

please rename to updateToolbarStyle so it's a bit clearer what it does

::: browser/themes/linux/browser.css
@@ +658,5 @@
>    -moz-image-region: rect(0px 72px 24px 48px);
>  }
>  
> +#bookmarks-menu-button.bookmark-item {
> +  list-style-image: url("chrome://browser/skin/Toolbar-small.png");

is not this missing the moz-image-region?
in the original patch it was removed from below

   11.85 -toolbar[iconsize="small"] #bookmarks-button,
   11.86 -toolbar[iconsize="small"] #bookmarks-menu-button,
   11.87 -#bookmarks-menu-button.bookmark-item {
   11.88 +toolbar[iconsize="small"] #bookmarks-button {
   11.89    -moz-image-region: rect(0px 48px 16px 32px);
   11.90  }

Comment 20

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [:mak] from comment #19)
> ::: browser/base/content/browser-places.js
> @@ +1106,3 @@
> >    },
> >  
> > +  _updateStyle: function BUI__updateStyle() {
> 
> please rename to updateToolbarStyle so it's a bit clearer what it does

but keep the underscore prefix

Comment 21

[Marco Bonardo [:mak]]

when moving the button to the tabbar on Windows, the dropdown arrow is too close to the icon.

Comment 22

[Marco Bonardo [:mak]]

There is also something strange in the rules that hide the button, more particularly we were previously moving the button inside personal-bookmarks, while now we move it directly to the toolbar, so #personal-bookmarks should be removed from those 2 rules.
I think we should also add a couple rules to hide the button in customize mode if the menubar is autohidden, by using #wrapper-bookmarks-menu-button so we reduce the surprise when the user moves the button and then it disappears when customization is done.

Comment 23

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

(In reply to Marco Bonardo [:mak] from comment #21)
> when moving the button to the tabbar on Windows, the dropdown arrow is too
> close to the icon.

Hm. I just compared the current state of the patch with Firefox 20 on Windows 7, and the dropdown marker looks like its in the same spot. Or did we want to use this as an opportunity to space them out a little?

http://i.imgur.com/MyIfFv8.png (the background window is release, foreground is Nightly with my patch).

Comment 24

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch v1.5

(In reply to Marco Bonardo [:mak] from comment #22)
> There is also something strange in the rules that hide the button, more
> particularly we were previously moving the button inside personal-bookmarks,
> while now we move it directly to the toolbar, so #personal-bookmarks should
> be removed from those 2 rules.

Ok, I've removed #personal-bookmarks from those rules. Seems to behave correctly.

> I think we should also add a couple rules to hide the button in customize
> mode if the menubar is autohidden, by using #wrapper-bookmarks-menu-button
> so we reduce the surprise when the user moves the button and then it
> disappears when customization is done.

Maybe I misunderstand, but I'll assume you meant to hide the button in customize mode if the menubar is *not* autohidden. I've added that rule.

I've also changed the name of _updateStyle as requested.

Comment 25

[Marco Bonardo [:mak]]

(In reply to Mike Conley (:mconley) from comment #23)
> (In reply to Marco Bonardo [:mak] from comment #21)
> > when moving the button to the tabbar on Windows, the dropdown arrow is too
> > close to the icon.
> 
> Hm. I just compared the current state of the patch with Firefox 20 on
> Windows 7, and the dropdown marker looks like its in the same spot. Or did
> we want to use this as an opportunity to space them out a little?

ok, nevermind, I was misremembering. it can stay broken considered will likely be fixed in Australis.


(In reply to Mike Conley (:mconley) from comment #24)
> > I think we should also add a couple rules to hide the button in customize
> > mode if the menubar is autohidden, by using #wrapper-bookmarks-menu-button
> > so we reduce the surprise when the user moves the button and then it
> > disappears when customization is done.
> 
> Maybe I misunderstand, but I'll assume you meant to hide the button in
> customize mode if the menubar is *not* autohidden. I've added that rule.

yes, sorry, I inverted the logic.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 747494 [details] [diff] [review]
Patch v1.5

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

please file a bug in mozmill to upgrade their test checking the star button (should be a mere s/BookmarksMenuButton/BookmarkingUI/)

Thank you very much for quick reaction on this patch :)

::: browser/base/content/browser.css
@@ +311,5 @@
> +%ifdef MENUBAR_CAN_AUTOHIDE
> +#toolbar-menubar:not([autohide="true"]) ~ #nav-bar > #bookmarks-menu-button,
> +#toolbar-menubar:not([autohide="true"]) ~ toolbar > #bookmarks-menu-button,
> +#toolbar-menubar:not([autohide="true"]) > #bookmarks-menu-button,
> +#toolbar-menubar:not([autohide="true"])[customizing="true"] ~ toolbar > #wrapper-bookmarks-menu-button {

I don't think you need to check customizing, since #wrapper-bookmarks-menu-button exists only while customizing...
you should also hide the wrapper on the menubar itself (third rule).
actually, doesn't ~ toolbar already handle the ~ #nav-bar rule so that the first rule can be removed?

Comment 27

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

(In reply to Marco Bonardo [:mak] from comment #26)
> Comment on attachment 747494 [details] [diff] [review]
> Patch v1.5
> 
> Review of attachment 747494 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please file a bug in mozmill to upgrade their test checking the star button
> (should be a mere s/BookmarksMenuButton/BookmarkingUI/)

Can do.

> 
> Thank you very much for quick reaction on this patch :)
> 

And thank you for the quick reviews!

> ::: browser/base/content/browser.css
> @@ +311,5 @@
> > +%ifdef MENUBAR_CAN_AUTOHIDE
> > +#toolbar-menubar:not([autohide="true"]) ~ #nav-bar > #bookmarks-menu-button,
> > +#toolbar-menubar:not([autohide="true"]) ~ toolbar > #bookmarks-menu-button,
> > +#toolbar-menubar:not([autohide="true"]) > #bookmarks-menu-button,
> > +#toolbar-menubar:not([autohide="true"])[customizing="true"] ~ toolbar > #wrapper-bookmarks-menu-button {
> 
> I don't think you need to check customizing, since
> #wrapper-bookmarks-menu-button exists only while customizing...

Good point, fixed.

> actually, doesn't ~ toolbar already handle the ~ #nav-bar rule so that the
> first rule can be removed?

You're right! Removed redundant rule.

> you should also hide the wrapper on the menubar itself (third rule).

The problem with this is that it makes it possible to remove the item if we've accidentally dragged it into the menubar, since it will disappear as soon as it lands in the toolbar-menubar. Am I OK to land without this?

Comment 28

[Marco Bonardo [:mak]]

just ignore my #wrapper requests, let's keep this as sucky as it was before we changed it.

Comment 29

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Patch 1.6 (r+'d by mak)

Thanks Marco!

Comment 30

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Filed bug 870810 for Mozmill tests.

Comment 31

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

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

I had to de-bitrot this before landing. This is the patch I landed on mozilla-inbound as

https://hg.mozilla.org/integration/mozilla-inbound/rev/bed2e2a10c13

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 33

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

The Aurora release notes at http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/ talk about the bookmark star button, but this change was backed out. Who do I need to contact to have that changed?

Comment 34

[Justin Dolske [:Dolske]]

Any of the release drivers should be able to help with that (akeybl/lsblakk/bajaj).

Comment 35

[u279076]

Adding this to the sign-off criteria for Firefox 23.0b1.

Comment 36

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130620 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130620 Firefox/23.0

The star button is displayed as in the previous Firefox versions. Both it's UI and functionalities are the same as before (white star for non-bookmarks, yellow star for bookmared pages, mark as bookmark on first click, open dialog on second click - dialog also work fine, etc).