734326 - Use Australis button styling for bookmarks toolbar items on Windows & Linux

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P3)

Description

[Siddhartha Dugar [:sdrocking]]

The styling for :hover and [open="true"] states of the bookmarks toolbar items looks inconsistent with the remaining theme and is outdated. We should update these to match the appearance of other buttons on the nav-bar.

Comment 1

[Siddhartha Dugar [:sdrocking]]

I made a userstyle for testing this - http://userstyles.org/styles/46277/

Comment 2

[u428464]

Should become a priority with bug 734373 resolved.

Comment 3

[Siddhartha Dugar [:sdrocking]]

I am not sure how exactly this should be handled. The other buttons when placed on the Bookmarks toolbar look like bookmarks themselves. So there is this consistency which should be maintained. IIRC they use the styling "-moz-appearance: toolbarbutton" which I think is outdated. Updating it will fix this bug as well as bug 736179.

For those interested in the Australis style for bookmarks, my style mentioned in comment 1 has been updated to what we have on m-c.

Comment 4

[u428464]

Why this bug is still marked as unconfirmed since the hover/pressed styling for the bookmark toolbars and other UI areas should match Australis button styling.

Comment 5

[Justin Dolske [:Dolske]]

Is this bug still relevant?

Comment 6

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

Yes, it's still relevant. If the user has bookmarks in their bookmarks toolbar and the bookmarks toolbar is visible, the hover and active state of these buttons is inconsistent.

Comment 8

[u428464]

There are two things to be done here since it has be merged with bug 775191 :

1 Unify the styling of items in the bookmarks toolbar (i.e. the bookmarks place item and all the widgets placed here)
2. Make the bookmarks place item take the Australis button styling when placed outside the bookmarks toolbar.

Comment 9

[Kohei Yoshino [:kohei]]

I was surprised to see this issue even in Beta launched yesterday. Bookmark items look bad when a theme is applied.

Comment 10

[Kohei Yoshino [:kohei]]

Looks like my issue is Bug 984170.

Comment 11

[desiradaniel2007]

I can work on this. Seems pretty straight forward to fix given the stylesheet :)

Comment 12

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

Cool, thanks for helping out. You can ask any questions you have on IRC at irc.mozilla.org on channel #fx-team.

Comment 13

[u428464]

Hey Daniel anything new about this ?

Comment 14

[desiradaniel2007]

I started investigating the codebase yesterday (through MXR) and got some questions:

1. I am unable to build on either of the machines/OSes I have. Can I just edit omni.ja to see whether it works before committing?

2. How can I open omni.ja? (Windows)

However, it seems like I can reuse the code for that style and what is already in Australis as that user style still works fine.

Comment 15

[desiradaniel2007]

Lists from Live Bookmarks and folders do not have the Australis style even with the user style applied. I can go about it with this bug too. (I can edit bugs if necessary.)

The file I need to edit seems to be /browser/base/content/browser.css.

I will go with editing and testing the user style and then adding code to browser.css in whatever order it seems to be following. So please, let me know what happens once you build with the patch which I should be submitting tomorrow.

Comment 16

[u428464]

(In reply to Guillaume C. [:ge3k0s] from comment #8)
> There are two things to be done here since it has be merged with bug 775191 :
> 
> 1 Unify the styling of items in the bookmarks toolbar (i.e. the bookmarks
> place item and all the widgets placed here)
> 2. Make the bookmarks place item take the Australis button styling when
> placed outside the bookmarks toolbar.

Apart from that also note that there are at least three different platform styling :
-OSX 10.6-10.9
-Windows XP-7
-Windows 8 

AFAIK Linux uses one of the Windows styling but I don't know which one.

Comment 17

[desiradaniel2007]

I have looked into that userstyle and removed some rules which seem to no longer be needed. I think I could also remove transition: none declarations, as they do not seem to have any effect and un-prefix some stuff in there.

Would you please look into my fork please, so that I can release a patch? http://userstyles.org/styles/100229/australis-bookmark-items

Another thing which I would like you to answer is whether this bug covers items within bookmark toolbar menu items.

Comment 18

[u428464]

(In reply to desiradaniel2007 from comment #17)
> Another thing which I would like you to answer is whether this bug covers
> items within bookmark toolbar menu items.

I think so, and also the styling of bookmarks items outside the bookmarks toolbar.

Comment 19

[u428464]

Just a few things I noticed while using the userstyle :

1. The bookmarks overflow "double arrow" still uses the old styling.
2. When you put the bookmarks item in the navbar the styling used is the Australis one which is good, but the first item has its hover a bit cut off.
3. It still misses the new styling for other widgets in the bookmarks toolbar.

Comment 20

[u428464]

Hey Daniel any news about your patch ?

Comment 21

[desiradaniel2007]

(In reply to Guillaume C. [:ge3k0s] from comment #20)
> Hey Daniel any news about your patch ?

Hi Guillaume, I am very busy at the moment and anyone should feel free to continue working on it. However, if nobody does, I will continue work on this in summer where I will be able to finish it off quickly.

P.S.: Frontend really needs to get proper devtools to inspect the *whole* UI, since as it is, it involves looking into a bunch of code.

Thanks.

Comment 22

[Brandon Cheng]

(In reply to desiradaniel2007 from comment #21)
> P.S.: Frontend really needs to get proper devtools to inspect the *whole*
> UI, since as it is, it involves looking into a bunch of code.

Have you tried DOM Inspector? Or pulling open the Developer Tools on chrome://browser/content?

Comment 23

[desiradaniel2007]

(In reply to Brandon Cheng from comment #22)
> (In reply to desiradaniel2007 from comment #21)
> > P.S.: Frontend really needs to get proper devtools to inspect the *whole*
> > UI, since as it is, it involves looking into a bunch of code.
> 
> Have you tried DOM Inspector? Or pulling open the Developer Tools on
> chrome://browser/content?

Yes, of course but you cannot inspect bookmark items that way.

Comment 24

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I didn't find a spec for this anywhere, so it might need a UX bug in addition to the implementation work. Stephen, do you have anything flying around for this?

Comment 25

[u428464]

(In reply to Philipp Sackl [:phlsa] from comment #24)
> I didn't find a spec for this anywhere, so it might need a UX bug in
> addition to the implementation work. Stephen, do you have anything flying
> around for this?

Does this bug really need a spec ? It's pretty simple : apply the toolbar's buttons styling to the bookmarks item (inside and outside the bookmarks toolbar) and to all the widgets put into the bookmarks toolbar.

Comment 26

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #25)
> (In reply to Philipp Sackl [:phlsa] from comment #24)
> > I didn't find a spec for this anywhere, so it might need a UX bug in
> > addition to the implementation work. Stephen, do you have anything flying
> > around for this?
> 
> Does this bug really need a spec ?

Yes.

> It's pretty simple : apply the toolbar's
> buttons styling to the bookmarks item (inside and outside the bookmarks
> toolbar) and to all the widgets put into the bookmarks toolbar.

What padding should apply? Should the bar be as big as the navbar? I don't think so...
What do we do with the fact that the icons are smaller?
Do we use grayscale icons for the (live) bookmark folders to match the other things, or do we keep color so that favicons don't stand out so much?
Do we need to do the same trick we do for the nav bar on Windows and Linux to make the hit area bigger?
Does the border radius and border style stay the same?
How do we style folders (which have a dropdown) ?
Do we do subfolders or do we make them open the library?
If we do subfolders, how does that impact our design choices for the dropdowns themselves? Should we followup that instead of trying to do the arrowpanel styling in here?

Comment 27

[u428464]

Ok you just convinced me. A simple first step would be to apply the styling as seen in the userstyle from comment 17. 

There should also be followups concerning the icons or the menu styling. This one is really just about the hover styling.

Comment 28

[desiradaniel2007]

I have by now gathered enough information to continue working on this. However, if UX people think a review is necessary, please let me know of any changes which need to be applied.

Comment 29

[u428464]

Spec is here : http://people.mozilla.org/~mmaslaney/bookmark/Bookmark-toolbar-button-style.png

Comment 30

[Daniel Desira]

Now that the spec is clear, I have started working on it and should be reporting back with a patch soon!

Comment 31

[:Gijs (he/him)]

(looking at the spec: note that we ideally shouldn't set the height in ways that don't work with different font sizes on e.g. windows and linux, where font size is configurable, but that we should match the spec given the existing font size)

Comment 32

[desiradaniel2007]

(In reply to :Gijs Kruitbosch from comment #31)
> (looking at the spec: note that we ideally shouldn't set the height in ways
> that don't work with different font sizes on e.g. windows and linux, where
> font size is configurable, but that we should match the spec given the
> existing font size)

Agreed!

By the way, is there any guide on working with these massive stylesheets? I can hardly get to override a value in there. Maybe, I have some more learning to do before I am up to it.

Comment 33

[desiradaniel2007]

Some progress, but no idea where to cater for lightweight-themes. Got an idea on Windows and OS X specifics.

Comment 34

[desiradaniel2007]

Attachment: bookmarkitems.css

This is what I have done so far. I will apply values in the correct pseudoclasses within existing styles once I'm done and do a patch.

Would like some feedback on the height issue.

Comment 35

[u428464]

You should really redirect this to someone else. Maybe Gijs could have a look.

Comment 36

[:Gijs (he/him)]

Comment on attachment 8502740 [details]
bookmarkitems.css

Today is a travel day, and I was off yesterday afternoon. Sorry, I hope to get to this on Monday.

Comment 37

[desiradaniel2007]

(In reply to :Gijs Kruitbosch from comment #36)
> Comment on attachment 8502740 [details]
> bookmarkitems.css
> 
> Today is a travel day, and I was off yesterday afternoon. Sorry, I hope to
> get to this on Monday.

No worries! By the way, I am using Style Editor from Browser Toolbox to preview changes.

Comment 38

[desiradaniel2007]

Attachment: bookmarkitems.css

Lightweight theme detection and more accurate placement of bookmark-item text.

Comment 39

[:Gijs (he/him)]

Comment on attachment 8503689 [details] [diff] [review]
bookmarkitems.css

Can you create a patch instead? (see: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch )

>toolbarbutton.bookmark-item > .toolbarbutton-icon {
>  margin-right: 0px;

This should probably be -moz-margin-end, but also, why do we need to mess with the icon/text margins here? I didn't think the design changed anything there?

>}
>
>toolbarbutton.bookmark-item > .toolbarbutton-text {
>  padding-left: 6px;

-moz-padding-start, and the same question as above.

>}
>
>toolbarbutton.bookmark-item {
>  margin-left: 16px !important;

Why are all of these !important? Also, this should be -moz-margin-start, and shouldn't actually be 16px because that's the text-to-icon distance in the unhovered case; when you look further down the mock, the distance between two hovered items is much less (in other words, some of the 16px will be in the border and padding inside the items, not in margin between the items). Finally, we should make sure that there isn't too much margin in front of the initial item.

>  /* Needs specs to be clear on height */
>  margin-top: 5px !important;
>  margin-bottom: 5px !important;

If I recall correctly, the current height is already 22px for default fonts. I don't think we should worry too much about changing the height or top/bottom margin compared to what it is now.

>  border: 1px solid white !important;

This probably shouldn't be non-transparent on the default theme, and the specs also don't show this as white? Not sure why you made it white.

>  border-radius: 5px;

The specs say 2px?

>}
>
>toolbarbutton.bookmark-item:-moz-lwtheme:hover {color: red;

What's up with the foreground colors here? And why do only the lwtheme cases get a hover and active style?

>  background-image: linear-gradient(-179deg, rgba(255, 255, 255, 0.50) 0%, rgba(255, 255, 255, 0.40) 100%) !important;
>}
>
>toolbarbutton.bookmark-item:-moz-lwtheme:active {color: pink;
>  background-image: linear-gradient(-179deg, rgba(255, 255, 255, 0.50) 0%, rgba(255, 255, 255, 0.40) 100%) !important;
>}

Comment 40

[u428464]

It would be nice to have a new patch here, the styling of bookmarks items is still inconsistent.

Comment 41

[u428464]

FWIW bug 891258 should normally land on Nightly soon. I don't know if it contains anything usable here.

Comment 42

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #41)
> FWIW bug 891258 should normally land on Nightly soon. I don't know if it
> contains anything usable here.

I think we can simply use the same approach as that bug.

Comment 43

[Tim Nguyen :ntim]

It seems he is not working on it anymore.

--
Even though it took me 13 patches to finish the other bug, the approach here is fairly easy, we just need to add bookmark-item to the list of selectors in browser.css. See the patch in bug 891258.

Comment 44

[desiradaniel2007]

Hi, I am sorry for abandoning this bug but I cannot do any development on Mozilla projects due to other priorities.

:ntim No, it's not all that straightforward. Check the spec and anything that works from my code. Good luck.

Comment 45

[u428464]

Hey Tim could work on this ?

Comment 46

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #45)
> Hey Tim could work on this ?

I won't have the time to work on this, but if anyone wants to, I can mentor this.

Comment 47

[Ken Tian]

Hey, I would like to work on this. It'll be my first bug as well.

Comment 48

[Tim Nguyen :ntim]

(In reply to ken.yht from comment #47)
> Hey, I would like to work on this. It'll be my first bug as well.

Awesome !
You can find documentation on how to set up your development environment at http://codefirefox.com .

Once you've got everything set up :
- In browser/themes/(windows|osx|linux)/browser.css, you'll need to complete the selector list with .bookmark-item in the relevant places. Here are the relevant places in the windows file : [0], [1], [2], [3]. It should be similar in the osx and linux files.
- There's also a lightweight theme spec (see at the bottom of [4]), in this case, you should probably just add new rules in browser.css. You'll probably need to use the :-moz-lwtheme selector here. That can be done in a second part if needed.

Testing part :
Make sure the bookmarks toolbar button styling is similar to the toolbar button styling in the navbar.

Once you've done with the patch, submit it and flag it for feedback? to me or :Gijs.

[0] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#628
[1] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#671
[2] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#792
[3] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#841
[4] : http://people.mozilla.org/~mmaslaney/bookmark/Bookmark-toolbar-button-style.png

Comment 49

[Tim Nguyen :ntim]

Also, if you need help, feel free to needinfo? me here. You can also join the #introduction channel on IRC if you need help setting up your dev environment.

Comment 50

[Ken Tian]

Thanks for the pointers Tim. I already have my dev environment set up, so I'll check out your links and start working on this.

Comment 51

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #48)
> (In reply to ken.yht from comment #47)
> > Hey, I would like to work on this. It'll be my first bug as well.
> 
> Awesome !
> You can find documentation on how to set up your development environment at
> http://codefirefox.com .
> 
> Once you've got everything set up :
> - In browser/themes/(windows|osx|linux)/browser.css, you'll need to complete
> the selector list with .bookmark-item in the relevant places. Here are the
> relevant places in the windows file : [0], [1], [2], [3]. It should be
> similar in the osx and linux files.
> - There's also a lightweight theme spec (see at the bottom of [4]), in this
> case, you should probably just add new rules in browser.css. You'll probably
> need to use the :-moz-lwtheme selector here. That can be done in a second
> part if needed.

This is likely not enough, in that you'll need to be discerning in what rules with .bookmark-item can be removed from the existing stylesheets to remove the existing specialcased bookmarks toolbar styling.

Comment 52

[u428464]

Can this be marked as [qx] ? ;-)

Comment 53

[Blake Winton (:bwinton) (:☕️)]

I'm not sure.  Philipp, what do you think?

Comment 54

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Spec

Marking as QX :)
I attached the spec so that it's easier to find.

Comment 55

[u428464]

That would be nice to add this to the whiteboard then. ;-)

Comment 56

[Charles Milette]

I could do it, but I don't have admin rights on my computer, so I can't set up an build environnement...
I'll try to convince my parents for setting the build environnement.
While, I've realised a workaround using Stylish: https://userstyles.org/styles/111188/firefox-better-bookmarks

Comment 57

[Charles Milette]

Finally, I can't download the stuff since I have not so much bandwidth.

Comment 58

[Charles Milette]

Attachment: Patch for Windows

It's only for Windows and I haven't tested it, since I got trouble compiling Firefox.

Comment 59

[:Gijs (he/him)]

Comment on attachment 8575252 [details] [diff] [review]
Patch for Windows

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

This in itself is almost certainly not enough, unfortunately.

Have you tried getting help with your build problems in #introduction or #build on Mozilla's IRC network? ( https://wiki.mozilla.org/IRC )

Comment 60

[Tim Nguyen :ntim]

Comment on attachment 8575252 [details] [diff] [review]
Patch for Windows

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

By targetting .toolbarbutton-text, only the text will appear inside the button border, and we also want to see the icon in it.

Also, as Gijs said, you can ask in #introduction on IRC if you need help building Firefox.

Comment 61

[Tim Nguyen :ntim]

Attachment: Patch

This might sound unusual, but I haven't tested this on Windows.
Also, this doesn't match the lightweight theme spec yet.

Comment 62

[Tim Nguyen :ntim]

Attachment: Patch

Lots of things were broken in the previous patch. Fixed them.
Only tested on Windows and OSX.

Comment 63

[Tim Nguyen :ntim]

Attachment: Patch

Fixed issue on Windows.

Comment 64

[Tim Nguyen :ntim]

Attachment: Patch

Lightweight theme code for dropdown arrow on OSX

Comment 65

[Charles Milette]

If there is specifications for the lightweight themes, the toolbar buttons should be the same since the goal of this bug is to give the same styling to bookmarks and toolbar buttons...

Comment 66

[:Gijs (he/him)]

Comment on attachment 8588473 [details] [diff] [review]
Patch

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

This fails to apply against fx-team tip.

Looking at the patch, I wonder if it wouldn't be easier to change the JS involved to generate buttons that have toolbarbutton-1 (in addition to .bookmark-item, which we'll need to toggle the text/icon visibility).

::: browser/themes/linux/browser.css
@@ +121,5 @@
>  /* Places toolbar */
>  toolbarbutton.bookmark-item:not(.subviewbutton),
>  #personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
> +  margin: 2px;
> +  padding: 0 3px !important;

I don't think we should be adding !important here.

::: browser/themes/osx/browser.css
@@ +567,5 @@
>    transition-property: background, border-color;
>    transition-duration: 250ms;
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton),

Why is this necessary?

::: browser/themes/windows/browser.css
@@ +405,5 @@
>  
>  toolbarbutton.bookmark-item:not(.subviewbutton),
>  #personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
> +  margin: 2px;
> +  padding: 0 3px !important;

Let's not start setting !important if we didn't need to before.

@@ +616,5 @@
>  .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>    -moz-margin-end: 0;
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton),

Why is this necessary? We override everything again further down... :-\

@@ +660,5 @@
>    -moz-padding-start: 0;
>    -moz-box-align: center;
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton),

And this?

Comment 67

[Dão Gottwald [:dao]]

This doesn't seem Windows 10 specific and generally out of scope for the theme work we're doing there.

Comment 68

[Charles Milette]

It might be a good idea to think about devedition themes, since the margin isn't the same. (Especially up and down, these two are smaller, it is evident when switching to devedition theme

Comment 69

[Dão Gottwald [:dao]]

Not blocking bug 1192839 any longer. This is too big for our limited scope for Firefox 41.

Comment 71

[Scott Wu [:scottwu]]

I'm interested in taking this bug.

Hi Gijs, would you say the patch from Tim is close to complete? or would it be better to start from scratch? I saw your latest review mentioning it might be better to change the JS instead, which I don't really follow.

Thanks a lot!

Comment 72

[:Gijs (he/him)]

(In reply to Scott Wu [:scottwu] from comment #71)
> I'm interested in taking this bug.
> 
> Hi Gijs, would you say the patch from Tim is close to complete? or would it
> be better to start from scratch? I saw your latest review mentioning it
> might be better to change the JS instead, which I don't really follow.
> 
> Thanks a lot!

Hi Scott. We were planning to use this bug for folks doing an Outreachy internship. I'll take it for now so it doesn't show up in queries as needing an owner. Sorry!

Maybe you could consider looking at bug 989659, bug 1000700 and bug 1111138 ?

Comment 73

[Scott Wu [:scottwu]]

No problem Gijs, I'll take a look at the bugs you referenced. Thanks!

Comment 74

[Rakhi(:rakhisharma)]

Attachment: Bug 734326 - Use Australis button styling for bookmarks toolbar items, r= Gijs.

Review commit: https://reviewboard.mozilla.org/r/59288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59288/

Comment 75

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ee4623c522

Comment 76

[:Gijs (he/him)]

Comment on attachment 8762816 [details]
Bug 734326 - Use Australis button styling for bookmarks toolbar items, r= Gijs.

Stephen, can you take a look at this? Try builds in comment 75.

Comment 77

[Tim Nguyen :ntim]

(In reply to Rakhi(:rakhisharma) from comment #74)
> Created attachment 8762816 [details]
> Bug 734326 - Use Australis button styling for bookmarks toolbar items, r=
> Gijs.
> 
> Review commit: https://reviewboard.mozilla.org/r/59288/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/59288/

This patch only fixes linux, is that intentional ? You can take a look at my old patch that supports all platforms.

Comment 78

[:Gijs (he/him)]

(In reply to Tim Nguyen :ntim from comment #77)
> (In reply to Rakhi(:rakhisharma) from comment #74)
> > Created attachment 8762816 [details]
> > Bug 734326 - Use Australis button styling for bookmarks toolbar items, r=
> > Gijs.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/59288/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/59288/
> 
> This patch only fixes linux, is that intentional ?

Yes.

Comment 79

[Rakhi(:rakhisharma)]

(In reply to Tim Nguyen :ntim from comment #77)
> (In reply to Rakhi(:rakhisharma) from comment #74)
> > Created attachment 8762816 [details]
> > Bug 734326 - Use Australis button styling for bookmarks toolbar items, r=
> > Gijs.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/59288/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/59288/
> 
> This patch only fixes linux, is that intentional ? You can take a look at my
> old patch that supports all platforms.

Yes, This patch only fixes Linux. I use Linux and this is the only system available for testing.

Comment 80

[Stephen Horlander [:shorlander]]

Attachment: Linux - Button Differences

This is looking good, thank you. I did notice two things:

1) the button hover state looks to be darker on the bookmarks bar than the main toolbar
2) when a folder is open and hovered it has a highlight appearance, it should just have the open state appearance (e.g. see bookmarks menu panel from the main toolbar)

Comment 81

[Rakhi(:rakhisharma)]

Attachment: Bug 734326 - Use Australis button styling for bookmarks toolbar items. r= Gijs.

Review commit: https://reviewboard.mozilla.org/r/62682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62682/

Comment 82

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=618085558538

Comment 83

[:Gijs (he/him)]

Comment on attachment 8768438 [details]
Bug 734326 - Use Australis button styling for bookmarks toolbar items. r= Gijs.

(In reply to Rakhi(:rakhisharma) from comment #82)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=618085558538

Stephen, can you re-ui-review this, this time also on Windows?

Comment 84

[Stephen Horlander [:shorlander]]

(In reply to :Gijs Kruitbosch from comment #83)
> Comment on attachment 8768438 [details]
> Bug 734326 - Use Australis button styling for bookmarks toolbar items. r=
> Gijs.
> 
> (In reply to Rakhi(:rakhisharma) from comment #82)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=618085558538
> 
> Stephen, can you re-ui-review this, this time also on Windows?

Looks good to me on Linux, but Windows seems to be exhibiting the same hover effect behavior as the last Linux build. See: http://people.mozilla.org/~shorlander/bugs/bug-734326-bookmarks-toolbar-button-hover-win10.webm

Comment 85

[Rakhi(:rakhisharma)]

Comment on attachment 8768438 [details]
Bug 734326 - Use Australis button styling for bookmarks toolbar items. r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62682/diff/1-2/

Comment 86

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59f2dae797fa

Comment 87

[:Gijs (he/him)]

Comment on attachment 8768438 [details]
Bug 734326 - Use Australis button styling for bookmarks toolbar items. r= Gijs.

Stephen, can you check if the updated patch looks good on Windows? 

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-3ab2e1a091005e9bb8f0a53c80fc66175feb6592/try-win32/

Comment 88

[Stephen Horlander [:shorlander]]

Comment on attachment 8768438 [details]
Bug 734326 - Use Australis button styling for bookmarks toolbar items. r= Gijs.

Looks good to me.

Comment 89

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/63c2b8192228
Use Australis button styling for bookmarks toolbar items on windows and linux, r=Gijs, ui-r=shorlander

Comment 90

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63c2b8192228

Comment 91

[Filippo]

Attachment: Screenshot.png

I have moved bookmarks toolbar items into the main toolbar and now these items have a different height than the toolbar buttons.
Is it possible to change the bookmarks toolbar item height in order to match the other buttons height?

Comment 92

[:Gijs (he/him)]

(In reply to Filippo from comment #91)
> Created attachment 8775048 [details]
> Screenshot.png
> 
> I have moved bookmarks toolbar items into the main toolbar and now these
> items have a different height than the toolbar buttons.
> Is it possible to change the bookmarks toolbar item height in order to match
> the other buttons height?

Maybe. Please file a new bug for this.

Comment 93

[Filippo]

(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #92)
> (In reply to Filippo from comment #91)
> > Created attachment 8775048 [details]
> > Screenshot.png
> > 
> > I have moved bookmarks toolbar items into the main toolbar and now these
> > items have a different height than the toolbar buttons.
> > Is it possible to change the bookmarks toolbar item height in order to match
> > the other buttons height?
> 
> Maybe. Please file a new bug for this.

Filed bug 1289698

Comment 94

[[e-mail]]

i am not sure, we need more information.