865316 - bookmarks dropmarker button has wrong border color when window is unfocused

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Attachment: Screenshot on OSX

STR:

0. Open Firefox
1. Focus another app, but leave the toolbar visible in the background

ER:
Border looks like that of the other buttons

AR:
Border on the dropmarker is much fainter than that on the other buttons, or even that of the left half of the star button itself.

Comment 2

[:Gijs (he/him)]

Attachment: Patch

So this fixes the discrepancy between the button and the dropmarker and makes them look nice and consistent. However, this whole selector block was added as part of the Lion styling update in bug 679708. I'm not sure if this is the right fix, and/or if this also needs to be changed for the disabled state. Dao/Stephen, can you let me know?

Comment 3

[Dão Gottwald [:dao]]

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

We still need to fade the arrow, right? The selector might just be missing "> .dropmarker-icon"...

Comment 4

[:Gijs (he/him)]

Attachment: Patch that fades the icon only

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 747394 [details] [diff] [review]
> Patch
> 
> We still need to fade the arrow, right? The selector might just be missing
> "> .dropmarker-icon"...

Ah, I thought it seemed too easy... You're right, of course.

For the disabled state, there are styles for a dropmarker to get a different icon (although it looks really awful - maybe because it's not a retina image? But it doesn't look like the ordinary arrow is, either... not sure what's up there) so I'm taking that out completely in this patch - setting the opacity in addition to the disabled icon there makes it pretty much invisible.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 747430 [details] [diff] [review]
Patch that fades the icon only

>   .toolbarbutton-1[disabled="true"] > .toolbarbutton-menu-dropmarker,
>-  .toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-dropmarker,

If the second selector isn't wanted, shouldn't the same be true for the first one?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 747430 [details] [diff] [review]
> Patch that fades the icon only
> 
> >   .toolbarbutton-1[disabled="true"] > .toolbarbutton-menu-dropmarker,
> >-  .toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-dropmarker,
> 
> If the second selector isn't wanted, shouldn't the same be true for the
> first one?

I don't know. I tried looking for an example, but it seems we hide it completely on all the browser chrome places I could find (alltabs button, social provider button) and custom style it on the others (bookmarks manager buttons, devtools). I'm happy to remove it if you think that's the right thing to do. :-)

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Comment on attachment 747430 [details] [diff] [review]
> > Patch that fades the icon only
> > 
> > >   .toolbarbutton-1[disabled="true"] > .toolbarbutton-menu-dropmarker,
> > >-  .toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-dropmarker,
> > 
> > If the second selector isn't wanted, shouldn't the same be true for the
> > first one?
> 
> I don't know. I tried looking for an example, but it seems we hide it
> completely on all the browser chrome places I could find (alltabs button,
> social provider button)

Well, for testing you can just undo the hiding for one of these buttons in your local tree.

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 747430 [details] [diff] [review]
> Patch that fades the icon only
> 
> >   .toolbarbutton-1[disabled="true"] > .toolbarbutton-menu-dropmarker,
> >-  .toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-dropmarker,
> 
> If the second selector isn't wanted, shouldn't the same be true for the
> first one?

This took a bit of digging (note: DOMI's "!important" marker is broken!), but the answer seems to be "no", because the first one doesn't get !important in the styling for the dropmarker, like the menubutton does here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbarbutton.css#100

TBH, it looks much better when the @2x image doesn't get overriden. So perhaps a better fix would be to remove the !important, but that sounds like asking for trouble...

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> toolbarbutton.css#100
> 
> TBH, it looks much better when the @2x image doesn't get overriden. So
> perhaps a better fix would be to remove the !important

Yep, please drop the !important. Also please completely get rid of the padding property in line 101 while you're at it.

Comment 10

[:Gijs (he/him)]

Attachment: Patch improving toolkit while we're at it

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> > toolbarbutton.css#100
> > 
> > TBH, it looks much better when the @2x image doesn't get overriden. So
> > perhaps a better fix would be to remove the !important
> 
> Yep, please drop the !important. Also please completely get rid of the
> padding property in line 101 while you're at it.

Like this, I presume?

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 748783 [details] [diff] [review]
Patch improving toolkit while we're at it

Yep, thanks.

Comment 12

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 748783 [details] [diff] [review]
> Patch improving toolkit while we're at it
> 
> Yep, thanks.

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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