Move the bookmark star button outside of the location bar

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jaws, Assigned: mak)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {addon-compat, user-doc-needed})

Trunk
Firefox 23
addon-compat, user-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 disabled)

Details

Attachments

(2 attachments, 13 obsolete attachments)

171.65 KB, patch
Details | Diff | Splinter Review
6.22 KB, patch
Details | Diff | Splinter Review
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

6 years ago
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?

Updated

6 years ago
Component: Theme → Toolbars
QA Contact: theme → toolbars

Updated

6 years ago
OS: Windows XP → All
(Assignee)

Comment 2

6 years ago
I can do this, provided I get a mockup of the expected interaction, will ask Stephen later.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(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?
(Assignee)

Comment 5

6 years ago
(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

6 years ago
CCing UX team to know about it.
(Assignee)

Comment 7

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

Comment 8

6 years ago
(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 ?
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
Created attachment 628277 [details] [diff] [review]
wip 0.1

please don't feedback the css changes, since I miss the graphics I left them half-done.
Attachment #628277 - Flags: feedback?(mano)
(Assignee)

Updated

6 years ago
Blocks: 754942
(Assignee)

Updated

6 years ago
Blocks: 581238
(Assignee)

Updated

6 years ago
Blocks: 582139
(Assignee)

Updated

6 years ago
Summary: Move the Bookmark button outside of the address bar → Move the bookmark star button outside of the location bar
(Assignee)

Updated

6 years ago
Blocks: 751417

Comment 11

6 years ago
Any progress on this ?
I think Marco is still waiting for icons.

Updated

6 years ago
Flags: needinfo?(shorlander)
Keywords: icon
Created attachment 673234 [details]
Star Button Icons

Sent these to Marco a while back. Attaching here and I also added a retina display version for OSX.
Flags: needinfo?(shorlander)

Updated

6 years ago
Keywords: icon
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #628277 - Flags: feedback?(mano)
(Assignee)

Comment 15

6 years ago
Created attachment 708368 [details] [diff] [review]
wip 0.2
Attachment #628277 - Attachment is obsolete: true

Comment 16

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

Comment 17

5 years ago
I honestly think it will just make central if it works as expected.
(Assignee)

Comment 18

5 years ago
Created attachment 708536 [details] [diff] [review]
wip 0.3

basically done on Mac, missing Windows and Linux
Attachment #708368 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 708540 [details] [diff] [review]
wip 0.4
Attachment #708536 - Attachment is obsolete: true
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).
(Assignee)

Comment 21

5 years ago
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)
(Assignee)

Comment 22

5 years ago
Created attachment 709536 [details] [diff] [review]
wip 0.5

winstripe and pinstripe mostly done, gnomestripe, Aero classic and XP to do.
Attachment #673234 - Attachment is obsolete: true
Attachment #708540 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #673234 - Attachment is obsolete: false
(Assignee)

Comment 23

5 years ago
Created attachment 710176 [details] [diff] [review]
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.
Attachment #709536 - Attachment is obsolete: true
Attachment #710176 - Flags: review?(mano)
(Assignee)

Comment 24

5 years ago
in case trychooser forgets to post here, builds will appear in:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-62be97fd5ab3
(Assignee)

Comment 25

5 years ago
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 :)
Attachment #710176 - Flags: ui-review?(shorlander)
(Assignee)

Comment 26

5 years ago
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 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.
(Assignee)

Comment 28

5 years ago
(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.
(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.
(Assignee)

Comment 30

5 years ago
(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.
I think I prefer stacking them vertically.

Comment 32

5 years ago
(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.
(Assignee)

Comment 33

5 years ago
(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

5 years ago
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)
(Assignee)

Comment 35

5 years ago
(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
(Assignee)

Updated

5 years ago
Blocks: 414771
Sorry to have been holding this up on ui-review. Screenshots, or, better, a build to try would help us to do the review.
(Assignee)

Comment 37

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
Thanks, that saves me a bunch of time :)
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.
Attachment #710176 - Flags: ui-review?(shorlander) → ui-review+
(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.
(Assignee)

Comment 42

5 years ago
(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.
(Assignee)

Comment 43

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

will unbitrot next week
Attachment #710176 - Flags: review?(mano)
Created attachment 732878 [details] [diff] [review]
patch v1.1

De-bitrotting for mozilla-central.
Attachment #710176 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 736002 [details] [diff] [review]
patch v1.2
Attachment #732878 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
Created attachment 736332 [details] [diff] [review]
patch v1.3

Tested across tier1 platforms, should be ready for review.
Will now generate some trybuilds.
Attachment #736002 - Attachment is obsolete: true
Attachment #736332 - Flags: review?(mano)
(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/mak77@bonardo.net-
> 67709ae62404/

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

Comment 49

5 years ago
(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 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.
(Assignee)

Comment 51

5 years ago
(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.
(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...
(Assignee)

Comment 53

5 years ago
Created attachment 736958 [details] [diff] [review]
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)
Attachment #673234 - Attachment is obsolete: true
Attachment #736332 - Attachment is obsolete: true
Attachment #736958 - Flags: review?(mano)
Attachment #736332 - Flags: review?(mano)

Comment 54

5 years ago
(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/mak77@bonardo.net-
> 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....
(Assignee)

Comment 55

5 years ago
(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 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?
Attachment #736958 - Flags: review?(mano) → review+
(Assignee)

Comment 57

5 years ago
(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
(Assignee)

Updated

5 years ago
Blocks: 863031
(Assignee)

Comment 58

5 years ago
Created attachment 739014 [details] [diff] [review]
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.
Attachment #739014 - Flags: review?(dao)
(Assignee)

Comment 59

5 years ago
Created attachment 739017 [details] [diff] [review]
Enlarge menubutton dropmarker

forgot to remove the linux devtools margin
Attachment #739014 - Attachment is obsolete: true
Attachment #739017 - Flags: review?(dao)
Attachment #739014 - Flags: review?(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?
(Assignee)

Comment 61

5 years ago
(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 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.
Attachment #739017 - Flags: review?(dao) → review+
(Assignee)

Comment 63

5 years ago
(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.
(Assignee)

Comment 64

5 years ago
Created attachment 740784 [details] [diff] [review]
patch v1.5
Attachment #736958 - Attachment is obsolete: true
(Assignee)

Comment 65

5 years ago
Created attachment 740785 [details] [diff] [review]
Increase menu-button dropmarker area
Attachment #739017 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1706d6e0514b
https://hg.mozilla.org/mozilla-central/rev/4002c6963c4e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Keywords: addon-compat, user-doc-needed
Depends on: 865211

Updated

5 years ago
Blocks: 865316
(Assignee)

Updated

5 years ago
Blocks: 865319

Updated

5 years ago
Depends on: 865350
Depends on: 865483

Updated

5 years ago
Depends on: 865668

Updated

5 years ago
Depends on: 865676

Comment 68

5 years ago
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.
Depends on: 866984

Updated

5 years ago
Depends on: 867224
relnote-firefox: --- → 23+
status-firefox23: --- → disabled
Flags: needinfo?(lsblakk)
The visible change was reverted in bug 867343.
This change was reverted, so flagging relnote ? for whenever it is re-landed.
relnote-firefox: 23+ → ?
Flags: needinfo?(lsblakk)
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.
(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

5 years ago
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

5 years ago
(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

5 years ago
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?
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?
Flags: needinfo?(mconley)
Flags: needinfo?(mak77)
Oh, sorry, I just got confused testing beta.

elbart: this change was undone in bug 867343 and won't ship until Australis does.
Flags: needinfo?(mconley)
Flags: needinfo?(mak77)
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.
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.
Flags: needinfo?(mak77)

Updated

5 years ago
Depends on: 904180

Updated

5 years ago
No longer depends on: 904180
(Assignee)

Comment 81

5 years ago
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.
Flags: needinfo?(mak77)

Comment 82

5 years ago
> 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.
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.
relnote-firefox: ? → ---

Updated

4 years ago
Depends on: 1014615
You need to log in before you can comment on or make changes to this bug.