1114570 - [devedition] Wrong searchbar go button icon

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

The go button should be an arrow, not a search glass.

The selector used to change the icon should be specific to the non-oneoffui : searchbar:not([oneoffui])

Comment 1

[:Gijs (he/him)]

Florian/Brian, not sure who knows about this.

Tim, can you provide more details? This is working for me in beta, and right now it's not clear to me whether this is devedition (theme) only, also on Nightly, a regression, needs specific steps to reproduce, or something else.

Comment 2

[Florian Quèze [:florian]]

My guess is this refers to http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/themes/shared/devedition.inc.css#l249

For the normal themes, we have 2 rules:
http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/themes/linux/searchbar.css#l57
http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/themes/linux/searchbar.css#l93

Comment 3

[:Gijs (he/him)]

Brian, want to have a look? I promise fast reviews! :-)

Comment 4

[Tim Nguyen :ntim]

This rule [0] should also be made specific.
[0] http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/themes/windows/devedition.css#l108

Comment 5

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #4)
> This rule [0] should also be made specific.
> [0]
> http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/themes/
> windows/devedition.css#l108

Nope, actually, this element doesn't exist for the oneoffui search bar.

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch

Comment 7

[:Gijs (he/him)]

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

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

We should fix the arrow for the oneoffui case, too.

Comment 8

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8540303 [details] [diff] [review]
> Patch
> 
> Review of attachment 8540303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should fix the arrow for the oneoffui case, too.

The arrow used by the oneoffui is the urlbar one, and there's a bug filed about the urlbar icon being too dark : bug 1095836 (which is waiting for assets).

Comment 9

[Tim Nguyen :ntim]

Should we wait for bug 1095836 or can we land this fix ?

Comment 10

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #9)
> Should we wait for bug 1095836 or can we land this fix ?

No, and no. We should fix this to use the same icon, then when the icon gets replaced, this fix will "just work".

Comment 13

[Brian Grinstead [:bgrins]]

Tim, could you update this patch as noted in Comment 10?

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > Should we wait for bug 1095836 or can we land this fix ?
> 
> No, and no. We should fix this to use the same icon, then when the icon gets
> replaced, this fix will "just work".

So, right now devedition isn't using any custom asset for that 'go' icon.  We are waiting for one in Bug 1095836 - there isn't anything to switch to right now.  So unless if I'm misunderstanding the request, I'd vote for landing this fix.

Comment 15

[:Gijs (he/him)]

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

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

We can land it because it's better than nothing, but it'd be good to get an updated asset for the (default) dark case... :-\

Comment 16

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/7a776d0c87e6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a776d0c87e6