748894 - Move the bookmark star button outside of the location bar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

As part of the new Australis design, the bookmark star should be moved outside of the address bar and placed next to it on the navigation toolbar.

Comment 1

[u428464]

There can currently be an issue with the search bar. In fact on every mockup search bar and address bar are merged and the star/bookmarks button is next to it. So will the merged star/bookmarks button be between the address bar and the search bar or next to the search bar on the right?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I can do this, provided I get a mockup of the expected interaction, will ask Stephen later.

Comment 3

[u428464]

Here is a first mockup, but it doesn't resolve the concern I had in comment 1 : http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html

Comment 4

[Dão Gottwald [:dao]]

(In reply to Guillaume C. [:ge3k0s] from comment #1)
> There can currently be an issue with the search bar. In fact on every mockup
> search bar and address bar are merged and the star/bookmarks button is next
> to it. So will the merged star/bookmarks button be between the address bar
> and the search bar or next to the search bar on the right?

The location bar and the search bar must remain adjacent, so putting it on the right side of the search bar is the only option.

(In reply to Marco Bonardo [:mak] from comment #2)
> I can do this, provided I get a mockup of the expected interaction, will ask
> Stephen later.

Anything wrong with just letting the button part act like the current star and the dropdown part like the current bookmarks button?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Marco Bonardo [:mak] from comment #2)
> > I can do this, provided I get a mockup of the expected interaction, will ask
> > Stephen later.
> 
> Anything wrong with just letting the button part act like the current star
> and the dropdown part like the current bookmarks button?

No, and as a first step that's what will happen. Though Faaborg had a bunch of ongoing work to change the interaction there and I wondered if the UX team still had plans to change it and mock-ups regarding those.

Comment 6

[u428464]

CCing UX team to know about it.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

just to mark progress, I have a wip patch, dnd and some code/css cleanup to do yet.

Comment 8

[u428464]

(In reply to Marco Bonardo [:mak] from comment #7)
> just to mark progress, I have a wip patch, dnd and some code/css cleanup to
> do yet.

Will it be pushed to the UX branch for testing ?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

It's a possibility, though from what I can tell it's working quite well and basically just moving and coalescing existing functionality, so a trybuild may even be enough.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 0.1

please don't feedback the css changes, since I miss the graphics I left them half-done.

Comment 11

[u428464]

Any progress on this ?

Comment 12

[Asaf Romano (gone)]

I think Marco is still waiting for icons.

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Star Button Icons

Sent these to Marco a while back. Attaching here and I also added a retina display version for OSX.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yeah, I got the icons already, I just have to complete the patch.
My only doubt was whether we wanted this change until we merge the locationbar and searchbar, since it will basically detach the star from the url.
Btw, coming soon.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 0.2

Comment 16

[u428464]

Since Australis is being pushed little by little on the UX branch, this patch could be landed here to when ready for testing.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I honestly think it will just make central if it works as expected.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 0.3

basically done on Mac, missing Windows and Linux

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 0.4

Comment 20

[Asaf Romano (gone)]

As they're not detailed anywhere, let's log here any UI changes that are not part of the initial design.

I'll start.

Yesterday we agreed to remove the "Bookmark This Page" menu item from the menu-button menu, the reasons being:
1) As it's the "default action" for the button now, it should have been, if at all, at the top of the menu. However, if we do that we'd also have to move up the Subscribe menu item, which isn't something we want to do.
2) Because toolbarbuttons aren't keyboard-accessible, we don't need a menu-item for the default action.

Other than that, we also figured it's somewhat buggy:
1) It's labeled "Bookmark This Page" even when the page is already bookmarked.
4) It's not disabled when the button is.

I'll soon file a follow up to fix the last couple of issues elsewhere (mac menus, keyboard shortcuts, maybe more).

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Recent changes in wip patches:
1. Removed Bookmark this page from the button menu, for the reasons stated in comment 20
2. If the star button is removed the panel will first try to anchor to the page-proxy-favicon in the locationbar, if this also fails fallback to content
3. small perf improvement after adding a bookmark (avoid one call to getMostRecentBookmarkForURI)

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip 0.5

winstripe and pinstripe mostly done, gnomestripe, Aero classic and XP to do.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

fixed gnomestripe and verified the others.

The only issue is native theming on Mac is corrupted (there is a vertical gradient line when the dropdown is pressed. This is something in native theming cocoa code that should be fixed apart.
Apart this, there may be need for some small design tweaks, that's why as soon as I can get a non-corrupted repository I'll make some trybuild for shorlander.

But we may already start the review process for the code part.

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

in case trychooser forgets to post here, builds will appear in:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-62be97fd5ab3

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 710176 [details] [diff] [review]
patch v1.0

ok the builds are there, rememeber they survive only a few days (3?) so please download them early.
I'm fine with delaying the fix a bit, or until FF22, just remember this patch bitrots easily and unbitrotting it takes quite some time :)

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I clearly have some tests to fix:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug422590.js | file menu is enabled after toolbar customization - Got true, expected false
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug432599.js | uncaught exception - ReferenceError: PlacesStarButton is not defined at chrome://mochitests/content/browser/browser/base/content/test/browser_bug432599.js:64
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | uncaught exception - ReferenceError: PlacesStarButton is not defined at chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js:38
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug624734.js | uncaught exception - ReferenceError: PlacesStarButton is not defined at chrome://mochitests/content/browser/browser/base/content/test/browser_bug624734.js:14

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 710176 [details] [diff] [review]
patch v1.0

>--- a/browser/themes/winstripe/browser-aero.css
>+++ b/browser/themes/winstripe/browser-aero.css
>@@ -174,29 +174,41 @@
>   #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child:not(:-moz-lwtheme) {
>     background-color: transparent !important;
>     color: black;
>     text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
>     border-left-style: none !important;
>     border-right-style: none !important;
>   }
> 
>-  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) > .toolbarbutton-icon:not(:-moz-lwtheme),
>-  #TabsToolbar[tabsontop=true] :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#new-tab-button,#sync-button[status])) > .toolbarbutton-icon:not(:-moz-lwtheme),
>-  #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#new-tab-button,#sync-button[status])) > .toolbarbutton-icon:not(:-moz-lwtheme) {
>+  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status],#bookmarks-menu-button)) > .toolbarbutton-icon:not(:-moz-lwtheme),
>+  #TabsToolbar[tabsontop=true] :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#new-tab-button,#sync-button[status],#bookmarks-menu-button)) > .toolbarbutton-icon:not(:-moz-lwtheme),
>+  #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#new-tab-button,#sync-button[status],#bookmarks-menu-button)) > .toolbarbutton-icon:not(:-moz-lwtheme) {
>     list-style-image: url("chrome://browser/skin/Toolbar-inverted.png");
>   }
> 
>   :-moz-any(#toolbar-menubar, #TabsToolbar[tabsontop=true], #nav-bar[tabsontop=false]) .toolbarbutton-1 > .toolbarbutton-menu-dropmarker:not(:-moz-lwtheme),
>   :-moz-any(#toolbar-menubar, #TabsToolbar[tabsontop=true], #nav-bar[tabsontop=false]) .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker:not(:-moz-lwtheme),
>   #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child .toolbarbutton-1 > .toolbarbutton-menu-dropmarker:not(:-moz-lwtheme),
>   #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker:not(:-moz-lwtheme) {
>     list-style-image: url("chrome://browser/skin/toolbarbutton-dropdown-arrow-inverted.png");
>   }
> 
>+  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) #bookmarks-menu-button,
>+  #TabsToolbar[tabsontop=true] #bookmarks-menu-button,
>+  #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child #bookmarks-menu-button {
>+    -moz-image-region: rect(18px 18px 36px 0px);
>+  }
>+
>+  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) #bookmarks-menu-button[starred],
>+  #TabsToolbar[tabsontop=true] #bookmarks-menu-button[starred],
>+  #nav-bar + #customToolbars + #PersonalToolbar[collapsed=true] + #TabsToolbar[tabsontop=false]:last-child #bookmarks-menu-button[starred] {
>+    -moz-image-region: rect(18px 36px 36px 18px);
>+  }

Why are you duplicating the logic here rather than putting the stars in Toolbar.png and Toolbar-inverted.png, respectively?

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

> .toolbarbutton-1[disabled=true] > .toolbarbutton-icon,
> .toolbarbutton-1[disabled=true] > .toolbarbutton-menu-dropmarker,
> .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-dropmarker,
>-.toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>+.toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+.toolbarbutton-1[type="menu-button"] > .toolbarbutton-menubutton-button[disabled] > .toolbarbutton-icon {
>   opacity: .4;
> }

[type="menu-button"] is redundant here.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #27)
> Why are you duplicating the logic here rather than putting the stars in
> Toolbar.png and Toolbar-inverted.png, respectively?

The button has multiple states, 2 but may become even 3, AFAIK Toolbar.png only supports one state, unless we want to enlarge it to have multiple states per button.

Comment 29

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #28)
> (In reply to Dão Gottwald [:dao] from comment #27)
> > Why are you duplicating the logic here rather than putting the stars in
> > Toolbar.png and Toolbar-inverted.png, respectively?
> 
> The button has multiple states, 2 but may become even 3, AFAIK Toolbar.png
> only supports one state, unless we want to enlarge it to have multiple
> states per button.

You may put as many stars as you like next to each other in Toolbar.png. They could also be stacked vertically, which would negatively affect the overall image size when you have three of them, but probably not enough to worry about it.

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #29)
> You may put as many stars as you like next to each other in Toolbar.png.
> They could also be stacked vertically

Do you have a preference?
The defect of the horizontal positioning is that, if in future we add more states, we have to update all of the other pixel indices following the star.
The defect of the vertical position is that increases the image size (as you said, the difference may be pointless, perf-wise)

I can do that at the next iteration, will surely simplify some of the css here.

Comment 31

[Dão Gottwald [:dao]]

I think I prefer stacking them vertically.

Comment 32

[u428464]

(In reply to Marco Bonardo [:mak] from comment #25)
> Comment on attachment 710176 [details] [diff] [review]
> patch v1.0
> 
> ok the builds are there, rememeber they survive only a few days (3?) so
> please download them early.

I'm repeating myself, but pushing this to the UX branch for testing could be a good thing.

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Guillaume C. [:ge3k0s] from comment #32)
> I'm repeating myself, but pushing this to the UX branch for testing could be
> a good thing.

The patch is a bit invasive and fails tests for now, there's still some work to do for those failures.

Comment 34

[Morpheus3k]

I have noticed that pressing the bookmark star button to open the bookmark panel is not reflected as a 'pressed state'. Pressing the new Download Button results in a pressed state while the Download Panel is open.
(tested on Windows 7 and OS X 10.8)

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Morpheus3k from comment #34)
> I have noticed that pressing the bookmark star button to open the bookmark
> panel is not reflected as a 'pressed state'. Pressing the new Download
> Button results in a pressed state while the Download Panel is open.
> (tested on Windows 7 and OS X 10.8)

it's probably some bug or different behavior in the menu-button widget, the downloads button is not a menu-button

Comment 36

[Madhava Enros [:madhava]]

Sorry to have been holding this up on ui-review. Screenshots, or, better, a build to try would help us to do the review.

Comment 37

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Madhava Enros [:madhava] from comment #36)
> Sorry to have been holding this up on ui-review. Screenshots, or, better, a
> build to try would help us to do the review.

I will do, but please notice in comment 25 (that I also notified on IRC) I asked to download the generated try builds before they disappeared, cause to generate new trybuilds I have to unbitrot all of the code everytime.

Comment 38

[Stephen Horlander [:shorlander]]

Uploading to here: http://people.mozilla.org/~shorlander/Bookmark-Star-Builds/

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thanks, that saves me a bunch of time :)

Comment 40

[Stephen Horlander [:shorlander]]

Comment on attachment 710176 [details] [diff] [review]
patch v1.0

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

Looks good, the only bug I noticed was the panel does something janky on OS X: http://people.mozilla.com/~shorlander/bugs/panel-jank.webm

Otherwise there are some concerns about the interaction with the new split button but we can iterate at the beginning of a new cycle.

Comment 41

[Matthew N. [:MattN]]

(In reply to Stephen Horlander from comment #40)
> Looks good, the only bug I noticed was the panel does something janky on OS
> X: http://people.mozilla.com/~shorlander/bugs/panel-jank.webm

I think this is bug 841308 which has since been fixed.

Comment 42

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Matthew N. [:MattN] from comment #41)
> (In reply to Stephen Horlander from comment #40)
> > Looks good, the only bug I noticed was the panel does something janky on OS
> > X: http://people.mozilla.com/~shorlander/bugs/panel-jank.webm
> 
> I think this is bug 841308 which has since been fixed.

It's possible, I should generate new try builds.
But also notice bug 792303 exists from a longer time and describes a similar behavior. It's hard to tell if both issues are gone or related as of now.

Comment 43

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 710176 [details] [diff] [review]
patch v1.0

will unbitrot next week

Comment 44

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

Attachment: patch v1.1

De-bitrotting for mozilla-central.

Comment 45

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

Comment 46

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.3

Tested across tier1 platforms, should be ready for review.
Will now generate some trybuilds.

Comment 47

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://tbpl.mozilla.org/?tree=Try&rev=67709ae62404

Builds in
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-67709ae62404/

Comment 48

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #47)
> https://tbpl.mozilla.org/?tree=Try&rev=67709ae62404
> 
> Builds in
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 67709ae62404/

The star icon moves one pixel upwards when bookmarking a page. Tested on Windows 7.

Comment 49

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #48)
> The star icon moves one pixel upwards when bookmarking a page. Tested on
> Windows 7.

Thanks, must be an alignment problem in Toolbar.png, I will check again all of the alignments.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 736332 [details] [diff] [review]
patch v1.3

>--- a/browser/themes/linux/browser.css
>+++ b/browser/themes/linux/browser.css

>+.toolbarbutton-1[disabled] > .toolbarbutton-icon,
>+.toolbarbutton-1[disabled] > .toolbarbutton-menu-dropmarker,
>+.toolbarbutton-1[disabled] > .toolbarbutton-menubutton-dropmarker,
>+.toolbarbutton-1[disabled] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+.toolbarbutton-1 > .toolbarbutton-menubutton-button[disabled] > .toolbarbutton-icon {
>+  opacity: .4;
>+}

>-#feed-button[disabled="true"] > .toolbarbutton-icon {
>-  opacity: .3;
>-}

The reason this was set for that specific button rather than for all buttons is that we don't want to reduce the opacity for stock icons, as they have dedicated images for the disabled state.

>--- a/browser/themes/windows/browser-aero.css
>+++ b/browser/themes/windows/browser-aero.css

>   :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) > .toolbarbutton-icon:not(:-moz-lwtheme),
>+  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) > toolbarbutton > .toolbarbutton-icon:not(:-moz-lwtheme),

How does this work? @primaryToolbarButtons@ doesn't include #bookmarks-menu-button, does it? :not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) is redundant since these buttons don't have inner toolbarbutton nodes.

Comment 51

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #50)
> The reason this was set for that specific button rather than for all buttons
> is that we don't want to reduce the opacity for stock icons, as they have
> dedicated images for the disabled state.

I see, will fix it.

> >+  :-moz-any(#toolbar-menubar, #nav-bar[tabsontop=false]) :-moz-any(@primaryToolbarButtons@):not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) > toolbarbutton > .toolbarbutton-icon:not(:-moz-lwtheme),

> How does this work? @primaryToolbarButtons@ doesn't include
> #bookmarks-menu-button, does it?

it does, why shouldn't it? When previously you asked me to move the icons in Toolbar.png I though you wanted to avoid specific rules for a single button so I made it like any other primary button.

> :not(:-moz-any(#alltabs-button,#tabview-button,#sync-button[status])) is
> redundant since these buttons don't have inner toolbarbutton nodes.

I didn't change the rules to avoid introducing subtle bugs, I can reduce those rules.

Comment 52

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #51)
> > How does this work? @primaryToolbarButtons@ doesn't include
> > #bookmarks-menu-button, does it?
> 
> it does, why shouldn't it?

I wrongly thought you changed the button's id attribute, but you didn't, you just removed the container...

Comment 53

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.4

Fixed toolbar.png alignment on Win, verified all of the others and addressed other Dao's comments.

I'm obsoleting the attached icons since those are outdated compared to those Stephen sent me during the last work week (that are used in latest versions of the patch)

Comment 54

[Alice0775 White]

(In reply to Marco Bonardo [:mak] from comment #47)
> https://tbpl.mozilla.org/?tree=Try&rev=67709ae62404
> 
> Builds in
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 67709ae62404/

I tried the build.

And I found a usability issue.

When I want to open a bookmark from new widget, I get a bookmarked panel sometimes.
Because the dropdown marker is very narrow than the Star, I clicked it wrong...
It is very annoying....

Comment 55

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Alice0775 White from comment #54)
> When I want to open a bookmark from new widget, I get a bookmarked panel
> sometimes.
> Because the dropdown marker is very narrow than the Star, I clicked it
> wrong...
> It is very annoying....

Yes, stephen already notified of this issue, though fixing small details ina a large patch doesn't work well, I hope we can land this soon so we can immediately start working on required polish.
I think for this case we need a larger dropdown, though it has not been decided yet if we should enlarge dropdown for just this widget or for all menu-buttons since all of them will have similar issues.

Comment 56

[Asaf Romano (gone)]

Comment on attachment 736958 [details] [diff] [review]
patch v1.4

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

::: browser/base/content/browser-places.js
@@ +256,5 @@
>     */  
>    bookmarkPage: function PCH_bookmarkPage(aBrowser, aParent, aShowEditUI) {
> +    if (aBrowser.contentWindow != window.content)
> +      throw new Error("trying to bookmark a non-current browser");
> +

What's this change for? This makes renders any caller but bookmarkCurrentPage invalid, right?

@@ +313,5 @@
> +
> +    // Try to dock the panel to:
> +    // 1. the bookmarks menu button
> +    // 2. the page-proxy-favicon
> +    // 3. the content area

I wonder if we should change the last fall-back to the current tab (well, unless the tabbar is hidden, i guess)

@@ +1002,3 @@
>  let BookmarksMenuButton = {
>    get button() {
> +    if (!this._button) {

Doesn't this introduce a js warning the first time it's called?

Anyway, I'm not sure it's worth caching. The final call is yours.

Same goes for the other getters.

::: browser/themes/osx/browser.css
@@ +1741,4 @@
>    }
>  
> +  #bookmarks-menu-button.bookmark-item > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
> +    width: 16px;

Why is this necessary?

Comment 57

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mano from comment #56)
> ::: browser/base/content/browser-places.js
> @@ +256,5 @@
> >     */  
> >    bookmarkPage: function PCH_bookmarkPage(aBrowser, aParent, aShowEditUI) {
> > +    if (aBrowser.contentWindow != window.content)
> > +      throw new Error("trying to bookmark a non-current browser");
> > +
> 
> What's this change for? This makes renders any caller but
> bookmarkCurrentPage invalid, right?

We discussed removing bookmarkPage, though I just found add-ons are using it, so it's not something I'm going to handle here.  I will just remove this check that is indeed changing the current behavior.
bookmarkPage is wrongly named for what is doing, cause it's indeed adding a bookmark for a given browser, though then it tries to open the star panel, even if the browser is not the current one. The example you made about the sidebar is pretty much concrete, you have a panel docked to the star that doesn't refer to the current browser :(
My fix was wrong though, we can handle it better, maybe by using the dialog instead of the star, in bookmarkPage.  Though not here, I will file a follow-up.

> @@ +313,5 @@
> > +
> > +    // Try to dock the panel to:
> > +    // 1. the bookmarks menu button
> > +    // 2. the page-proxy-favicon
> > +    // 3. the content area
> 
> I wonder if we should change the last fall-back to the current tab (well,
> unless the tabbar is hidden, i guess)

I think it's a neverending story, whatever we take make be hidden and we fallback, in the end we need to fallback to something that will very very likely exist, thus I think the content area is fine for now.

> ::: browser/themes/osx/browser.css
> @@ +1741,4 @@
> >    }
> >  
> > +  #bookmarks-menu-button.bookmark-item > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
> > +    width: 16px;
> 
> Why is this necessary?

the @2x icons

Comment 58

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Enlarge menubutton dropmarker

While I originally thought to leave this to a followup, I quickly figured the situation is terrible, especially on Linux and Mac.  We just can't release it as-is.
This is the first (and only) menu-button we have on toolbar, though also devtools have their own menubuttons in the console.  They indeed already tried to workaround the fact the toolkit theme is unusable, by adding a margin on Mac and Linux, if not that the Windows version is not so usable yet and the Mac rule doesn't work.
I think it's nicer if we make the dropmarker globally more usable, as it is now it's unlikely any user may be able to open the bookmarks menu. This improves usability, we can still evaluate further cleanup, for now I tried to limit changes to the bare minimum.  The scope is to make the dropdown clickable, but not as large as the main button.

A nice side effect of these changes is that they also solve the vertical graphical glitch on Mac menu-buttons...

Dao, I'd like to have your insight on these changes.

Comment 59

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Enlarge menubutton dropmarker

forgot to remove the linux devtools margin

Comment 60

[Dão Gottwald [:dao]]

Comment on attachment 739017 [details] [diff] [review]
Enlarge menubutton dropmarker

>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css

> .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
>-  width: 14px;
>-  padding-top: 2px;
>+  padding: 2px 4px 0;
>   -moz-border-start: none !important;
> }

Does this need to diverge from the toolkit styling?

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css

> @navbarLargeIcons@ .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
>-  padding: 8px 3px 7px;
>+  padding: 8px 5px 7px;
> }

Why this change? Was the padding you added for .toolbarbutton-menubutton-dropmarker in toolkit not enough?

>--- a/toolkit/themes/linux/global/toolbarbutton.css
>+++ b/toolkit/themes/linux/global/toolbarbutton.css

> .toolbarbutton-menubutton-dropmarker {
>   padding: 3px;
>+  margin: 0 3px;
>   -moz-appearance: toolbarbutton-dropdown !important;
>   list-style-image: none;
>   -moz-image-region: auto;
> }

Can you increase the padding here rather than adding a margin?

Comment 61

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #60)
> >--- a/browser/themes/osx/browser.css
> >+++ b/browser/themes/osx/browser.css
> 
> > .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> >-  width: 14px;
> >-  padding-top: 2px;
> >+  padding: 2px 4px 0;
> >   -moz-border-start: none !important;
> > }
> 
> Does this need to diverge from the toolkit styling?

there is a margin on the toolbar buttons, if I keep the 5px it becomes far too big, I think 4px is a good compromise to make it look like the Web console buttons.

> >--- a/browser/themes/windows/browser.css
> >+++ b/browser/themes/windows/browser.css
> 
> > @navbarLargeIcons@ .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
> >-  padding: 8px 3px 7px;
> >+  padding: 8px 5px 7px;
> > }
> 
> Why this change? Was the padding you added for
> .toolbarbutton-menubutton-dropmarker in toolkit not enough?

It is being overwritten in browser.css by a 2px padding, this adds a 5px padding to the icon that in the end makes it 7px (as toolkit).
It's lots of complication but these menu-button rules are all fragmented and I didn't want to move everything.

> >--- a/toolkit/themes/linux/global/toolbarbutton.css
> >+++ b/toolkit/themes/linux/global/toolbarbutton.css
> 
> > .toolbarbutton-menubutton-dropmarker {
> >   padding: 3px;
> >+  margin: 0 3px;
> >   -moz-appearance: toolbarbutton-dropdown !important;
> >   list-style-image: none;
> >   -moz-image-region: auto;
> > }
> 
> Can you increase the padding here rather than adding a margin?

Nope, this uses -moz-appearance and the padding seems to be ignored (I was unable to apply any padding afaict)

Comment 62

[Dão Gottwald [:dao]]

Comment on attachment 739017 [details] [diff] [review]
Enlarge menubutton dropmarker

>--- a/toolkit/themes/linux/global/toolbarbutton.css
>+++ b/toolkit/themes/linux/global/toolbarbutton.css
>@@ -104,12 +104,13 @@ toolbarbutton[type="menu-button"][disabl
>   -moz-box-pack: center;
>   -moz-box-orient: vertical;
> }
> 
> /* .......... dropmarker .......... */
> 
> .toolbarbutton-menubutton-dropmarker {
>   padding: 3px;
>+  margin: 0 3px;
>   -moz-appearance: toolbarbutton-dropdown !important;
>   list-style-image: none;
>   -moz-image-region: auto;
> }

Seems like you should remove the padding.

Comment 63

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #62)
> > .toolbarbutton-menubutton-dropmarker {
> >   padding: 3px;
> >+  margin: 0 3px;
> >   -moz-appearance: toolbarbutton-dropdown !important;
> >   list-style-image: none;
> >   -moz-image-region: auto;
> > }
> 
> Seems like you should remove the padding.

yep, unfortunately checking again I found devtools use -moz-appearance: none, that means for them the padding is needed :(
So I left only the margin: 0 3px; in toolkit and added padding: 0 3px; in devtools, to keep things consistent.

Comment 64

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.5

Comment 65

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Increase menu-button dropmarker area

Comment 66

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1706d6e0514b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4002c6963c4e

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1706d6e0514b
https://hg.mozilla.org/mozilla-central/rev/4002c6963c4e

Comment 68

[Armin Ronacher]

I really don't want to bikeshed on this one but is there any chance we can get the star back into the location bar?  I barely ever bookmark something so me clicking that button is not happening very often and the increased size basically made me remove it from my toolbar now.

Comment 69

[Matthew N. [:MattN]]

The visible change was reverted in bug 867343.

Comment 70

[Lukas Blakk [:lsblakk] use ?needinfo]

This change was reverted, so flagging relnote ? for whenever it is re-landed.

Comment 71

[bhavana bajaj [:bajaj]]

Spoke to make and this is disabled for FX24 as well so not considering for releasing noting in the upcoming aurora notes for Fx24.

He also noted no changes to bookmark UI until Australis.

Comment 72

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #71)
> Spoke to make and this is disabled for FX24 as well so not considering for
    whoops, meant :mak here

> releasing noting in the upcoming aurora notes for Fx24.
> He also noted no changes to bookmark UI until Australis.

Comment 73

[Elbart]

Regarding the bigger dropdownmarker-area for menu-buttons:
When the theme is set to "Small icons", the marker-area is now bigger than the actual icon, see http://i.imgur.com/Scf2eFC.jpg (top 22, bottom 23).
I'd consider this a regression.

Comment 74

[Dave Garrett]

(In reply to elbart from comment #73)
> Regarding the bigger dropdownmarker-area for menu-buttons:
> When the theme is set to "Small icons", the marker-area is now bigger than
> the actual icon, see http://i.imgur.com/Scf2eFC.jpg (top 22, bottom 23).
> I'd consider this a regression.

Note that eventually small icons mode is getting removed and such things are to be banished to individual themes. See bug 863299. (I'm not particularly fond of the idea, but that seems to be the plan)

Comment 75

[Elbart]

I know that "Small icons" will be removed with Australis. But Australis is still almost half a year away, or even longer if the ESR is being used.
What is the reason to mess with the old UI now, several months before the big change? 
Wouldn't UX be a more appropriate channel to land these UI-changes?

Comment 76

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

Hmm, I thought we had disabled this across the board, but it looks like that only happened on Aurora (and we've now shipped this change to Beta).

What's the plan here?

Comment 77

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

Oh, sorry, I just got confused testing beta.

elbart: this change was undone in bug 867343 and won't ship until Australis does.

Comment 78

[Elbart]

The changes of comment 65 were not undone and were released in 23,
see https://hg.mozilla.org/releases/mozilla-release/rev/4002c6963c4e/toolkit/themes/windows/global/toolbarbutton.css

Comment 79

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

Can you file a new bug about that, and CC me? Seems like maybe something worth fixing for 24, but tracking it in comments in a FIXED bug isn't ideal.

Comment 80

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

If I remember correctly, Marco decided to keep that change in, as it makes a larger click-target for the dropdown handle for the bookmarks button. I might be mistaken.

Comment 81

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The dropdown has been enlarged since it was barely usable, most of our ui (like devtools) was already enlarging it and the patch just unified that. We also didn't have any good examples of that kind of widget so far in the product, so it had never been evaluated for polish so far.
The mistake at a maximum is not having handled small-icons mode in browser. At this point we could handle it for 24, or ignore it, knowing it's going away. It may be considered a visual regression for small icons mode, though the fact nobody noticed it until now, makes me think it may not be so problematic.

Comment 82

[Armin Ronacher]

> though the fact nobody noticed it until now, makes me think it may not be so problematic.

This has been noticed before, see my comment:

> [...] and the increased size basically made me remove it from my toolbar now.

Comment 83

[Lukas Blakk [:lsblakk] use ?needinfo]

removing the relnote flag, since this is FIXED it will never make sense - we'll have to file a bug to re-enable it and relnote that one.