Closed Bug 867343 Opened 7 years ago Closed 7 years ago

Back out star button UI changes

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 23
Tracking Status
firefox23 + verified

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(1 file, 10 obsolete files)

47.77 KB, patch
mconley
: review+
Details | Diff | Splinter Review
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.
After bug 853071 lands, you could put this behind an #ifdef NIGHTLY_BUILD?
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.
So, my question is: why we can't take an intermediate solution while waiting for Australis, that regardless will change basically everything?
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.
(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.
(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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Summary: Back out star button changes after it hits Aurora channel → Back out star button UI changes
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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?
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attachment #746451 - Flags: feedback?(mak77)
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #746451 - Attachment is obsolete: true
Attachment #746451 - Flags: feedback?(mak77)
Attached patch WIP Patch 3 (obsolete) — Splinter Review
Adds styling for OSX.
Attachment #746458 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, that's all 3 OS's.

So what am I overlooking? And what have I forgotten to remove?
Attachment #746529 - Attachment is obsolete: true
Attachment #746573 - Flags: feedback?(mak77)
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
Attachment #746573 - Flags: feedback?(mak77)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #746573 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
Some follow-ups for OSX and Linux.
Attachment #746977 - Attachment is obsolete: true
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #746984 - Attachment is obsolete: true
Attached patch Patch v1.4 (obsolete) — Splinter Review
Getting rid of some dumps that shouldn't have been there.
Attachment #747005 - Attachment is obsolete: true
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?
Attachment #747006 - Flags: feedback?(mak77)
Hrm. browser_bug432599.js seems to only fail on my Ubuntu machine. Windows and OSX are just fine. :/
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  }
Attachment #747006 - Flags: feedback?(mak77) → feedback+
(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
when moving the button to the tabbar on Windows, the dropdown arrow is too close to the icon.
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.
(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).
Attached patch Patch v1.5 (obsolete) — Splinter Review
(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.
Attachment #747006 - Attachment is obsolete: true
Attachment #747494 - Flags: review?(mak77)
(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 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?
Attachment #747494 - Flags: review?(mak77) → review+
(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?
Flags: needinfo?(mak77)
just ignore my #wrapper requests, let's keep this as sucky as it was before we changed it.
Flags: needinfo?(mak77)
Attached patch Patch 1.6 (r+'d by mak) (obsolete) — Splinter Review
Thanks Marco!
Attachment #747494 - Attachment is obsolete: true
Attachment #747986 - Flags: review+
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
Attachment #747986 - Attachment is obsolete: true
Attachment #748001 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bed2e2a10c13
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 871281
Depends on: 871325
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?
Flags: needinfo?(dolske)
Any of the release drivers should be able to help with that (akeybl/lsblakk/bajaj).
Flags: needinfo?(dolske)
Adding this to the sign-off criteria for Firefox 23.0b1.
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: ioana.budnar
Depends on: 904180
Depends on: 924723
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.