Tim Nguyen :ntim Assignee
	Description • 10 years ago

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

	:Gijs (he/him)
	Comment 1 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 2 • 10 years ago

(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.

	Tim Nguyen :ntim Assignee
	Comment 3 • 10 years ago

(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

	Tim Nguyen :ntim Assignee
	Comment 4 • 10 years ago

(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.

	Tim Nguyen :ntim Assignee
	Comment 5 • 10 years ago

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;
> }

	Justin Dolske [:Dolske]
	Comment 6 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 7 • 10 years ago

(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.

	:Gijs (he/him)
	Comment 8 • 10 years ago

(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?

	Stephen Horlander [:shorlander]
	Comment 9 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 10 • 10 years ago

My first patch :)

	:Gijs (he/him)
	Comment 11 • 10 years ago

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

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

LGTM! Thanks for the patch. :-)

	Tim Nguyen :ntim Assignee
	Comment 12 • 10 years ago

(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.

	:Gijs (he/him)
	Comment 13 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 14 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 15 • 10 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 16 • 10 years ago

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

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

	Cornel Ionce [:noni] [Hubs QA]
	Comment 17 • 10 years ago

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).

	Justin Dolske [:Dolske]
	Comment 18 • 10 years ago

\o/, belatedly. Thanks for the patch!