984979 - Australis - Back button's open state doesn't match :active state on Windows 8

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

On Windows 8, the back button's active state is inconsistent with other button active states.

Comment 1

[:Gijs (he/him)]

I guess this is due to the shape, and by design. Stephen?

Comment 2

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #1)
> I guess this is due to the shape, and by design. Stephen?

It might just be the hover background overriding the active background.

Comment 3

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > I guess this is due to the shape, and by design. Stephen?
> 
> It might just be the hover background overriding the active background.

*for the back button

Comment 4

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #1)
> I guess this is due to the shape, and by design. Stephen?

I don't think so, the mockup shows a consistent background for all toolbar button active state.

Comment 5

[Tim Nguyen :ntim]

Turns out Mike forgot to add an active state for the back button.
http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#884

A fix would be :
>+#back-button:hover:active > .toolbarbutton-icon
> #back-button[open="true"] > .toolbarbutton-icon {
>    background-color: hsla(210,4%,10%,.12) !important;
>    box-shadow: 0 1px 0 0 hsla(210,80%,20%,.1) inset !important;
> }

Comment 6

[Justin Dolske [:Dolske]]

Patches welco...hey! :)

If you're not already, have you considered building locally and generating actual patches? It's the fastest way to help get nits like this fixed, and yay open source!

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 7

[Tim Nguyen :ntim]

(In reply to Justin Dolske [:Dolske] from comment #6)
> Patches welco...hey! :)
> 
> If you're not already, have you considered building locally and generating
> actual patches? It's the fastest way to help get nits like this fixed, and
> yay open source!
> 
> https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> 
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Ive done the codefirefox videos tutorials already, but I somehow screwed up my configuration. I'm currently looking into github development for this, since I'm more used to the environment.

Comment 8

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #7)
> (In reply to Justin Dolske [:Dolske] from comment #6)
> > Patches welco...hey! :)
> > 
> > If you're not already, have you considered building locally and generating
> > actual patches? It's the fastest way to help get nits like this fixed, and
> > yay open source!
> > 
> > https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> > 
> > https://developer.mozilla.org/en-US/docs/
> > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> > in_for_me.3F
> 
> Ive done the codefirefox videos tutorials already, but I somehow screwed up
> my configuration. I'm currently looking into github development for this,
> since I'm more used to the environment.

Alternatively, drop by in #introduction on http://irc.mozilla.org/ and we can try and help you?

Comment 9

[Stephen Horlander [:shorlander]]

So this appears to have the correct depressed state if the history menu is open, but not on :active.

Comment 10

[Tim Nguyen :ntim]

Attachment: bug_984979.patch

My first patch :)

Comment 11

[:Gijs (he/him)]

Comment on attachment 8395275 [details] [diff] [review]
bug_984979.patch

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

LGTM! Thanks for the patch. :-)

Comment 12

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8395275 [details] [diff] [review]
> bug_984979.patch
> 
> Review of attachment 8395275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM! Thanks for the patch. :-)

Thanks :) Gonna ask for checkin.

Comment 13

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/77802de7e213

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77802de7e213

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8395275 [details] [diff] [review]
bug_984979.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960517
User impact if declined: Awkward back button :active state
Testing completed (on m-c, etc.):  on m-c
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/88e7ab08a67e

https://hg.mozilla.org/releases/mozilla-beta/rev/b94841fb87d0

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0		
Mozilla/5.0 (Windows NT 6.2; rv:29.0) Gecko/20100101 Firefox/29.0		

Verified issue fixed using Firefox 29 beta 3 (build ID: 20140327113732), latest Aurora (build ID: 20140327004002) and latest Nightly (build ID: 20140327030203).

Comment 18

[Justin Dolske [:Dolske]]

\o/, belatedly. Thanks for the patch!