1091656 - Back button is missing a border on hover

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

This must have regressed this : https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7

Comment 1

[Brian Grinstead [:bgrins]]

I'm not seeing what would have caused it in that patch - there is nothing specifically targeting the back button.  Any ideas?

Comment 2

[Tim Nguyen :ntim]

This rule [0] is getting overriden now.
[0] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#962

Comment 3

[Brian Grinstead [:bgrins]]

What, by this: https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7#l1.101?  Hrm, I guess we could prefix this rule with #nav-bar?

Comment 4

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> What, by this:
> https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7#l1.101?  Hrm, I
> guess we could prefix this rule with #nav-bar?

Some people still put the navigation bar on other areas than the nav bar (with add-ons), so that wouldn't be a proper fix. The fix would be to introduce a !important on the box shadow property.

Comment 5

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > What, by this:
> > https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7#l1.101?  Hrm, I
> > guess we could prefix this rule with #nav-bar?
> 
> Some people still put the navigation bar on other areas than the nav bar
> (with add-ons), so that wouldn't be a proper fix. The fix would be to
> introduce a !important on the box shadow property.

This doesn't make any sense to me. How does swapping out the property value for a variable change what rule takes precedence? Those rules were there before and the selector hasn't changed.

Comment 6

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Tim Nguyen [:ntim] from comment #4)
> > (In reply to Brian Grinstead [:bgrins] from comment #3)
> > > What, by this:
> > > https://hg.mozilla.org/mozilla-central/rev/141414b0f7c7#l1.101?  Hrm, I
> > > guess we could prefix this rule with #nav-bar?
> > 
> > Some people still put the navigation bar on other areas than the nav bar
> > (with add-ons), so that wouldn't be a proper fix. The fix would be to
> > introduce a !important on the box shadow property.
> 
> This doesn't make any sense to me. How does swapping out the property value
> for a variable change what rule takes precedence? Those rules were there
> before and the selector hasn't changed.

Before the patch, there was no box-shadow set on hover for toolbar buttons, now, I need to set one, since aero uses one. And that rule, overrides the box shadow rule on the back button.

Comment 7

[Tim Nguyen :ntim]

Sorry, I meant set a box shadow variable.

Comment 8

[:Gijs (he/him)]

Ugh. I don't think sprinkling !important everywhere is the answer. For example, that's going to then require the devtools theme to do the same where they override box-shadow, and we'll need to add it for :hover:active too because otherwise that style gets overridden by :hover.

The simplest thing is to set the CSS variable to something invalid for the windows 8 case, like an empty string. That'll cause the property set to be ignored. Unfortunately, that'll also cause a warning in the console...

Alternatively, drop the variable-ifying for box-shadow and just override it in the devtools theme.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #8)
> The simplest thing is to set the CSS variable to something invalid for the
> windows 8 case, like an empty string. That'll cause the property set to be
> ignored. Unfortunately, that'll also cause a warning in the console...
> 

This, with a comment, would be my preference since then we don't have to keep a big selector list up to date in two places.

Comment 10

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #8)

Or maybe just override that variable for the back button ?
Like this :
#back-button {
  --toolbarbutton-hover-boxshadow: (back button box shadow)
}

Comment 11

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> 
> Or maybe just override that variable for the back button ?
> Like this :
> #back-button {
>   --toolbarbutton-hover-boxshadow: (back button box shadow)
> }

I could live with this, I guess.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: win8-boxshadow.patch

Does this fix it?

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8515022 [details] [diff] [review]
win8-boxshadow.patch

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

Gonna test this later today. The patch looks fine to me, but I'm sure there'll be a CSS error on Windows >= 8.

Comment 15

[Paul Rouget [:paul]]

I doesn't appear to solve the issue here (windows 10).

Comment 16

[Paul Rouget [:paul]]

So the back button is supposed to have a box-shadow:

>  box-shadow: 0 1px 0 0 hsla(210,4%,10%,.25),
>              0 0 0 1px hsla(210,4%,10%,.25);

Setting it to `none` makes the border disappear.

If we set the variable to this value, it will affect other buttons.

So we need the box-shadow to be ignored. We can't remove it as it's used by the black theme.

We can't make the variable an invalid value: an invalid value in a variable is transformed to "initial", it doesn't skip the value, for a reason I ignore. Proof: http://jsbin.com/ciyidikoca/1/edit?css,output

So basically, I think we need to create a new variable, or not use box-shadow in this rule.

Comment 17

[Tim Nguyen :ntim]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #16)
Or do what's suggested in comment 10

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8515022 [details] [diff] [review]
win8-boxshadow.patch

Paul seems to have answered your question.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: win8-boxshadow.patch

Uses approach from Comment 10.  I'm unable to test it at the moment (in the middle of a clobber build).  Can you confirm that it works?

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8515250 [details] [diff] [review]
win8-boxshadow.patch

This is not going to work, since the non-hover states also need the box-shadow as well.
So if you're going with this approach, you shouldn't remove the original box-shadow rule, but you could change it to this though (to avoid having the box-shadow written twice) :
#back-button > .toolbarbutton-icon {
  [...]
  box-shadow: var(--toolbarbutton-hover-boxshadow)
  [...]
}

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: win8-boxshadow.patch

I'm on Win7 so I tried to test this by removing some of the ifdefs but I'm fairly sure I didn't get the test right.  I don't have time at the moment to set up a Win8 development environment so could someone else please confirm if it is working as expected?

Gijs, let me know if you'd prefer to just keep a duplicate box-shadow value or reuse the variable as I'm doing here.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8515359 [details] [diff] [review]
win8-boxshadow.patch

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

Can't test this until tomorrow morning, but my midnight question here is...

::: browser/themes/windows/browser.css
@@ +960,5 @@
>    background-clip: padding-box !important;
>    background-color: hsla(210,25%,98%,.08) !important;
>    padding: 6px !important;
>    border-style: none !important;
> +  box-shadow: var(--toolbarbutton-hover-boxshadow);

This means that on the devtools theme, this will use the hover box-shadow all the time, right? Including when not hovered, and when hover:active?

Comment 23

[:Gijs (he/him)]

(fundamentally, it's kind of weird to have a variable named ...-hover-... that's used... all the time, not just on hover. I realize it needs to be the same name - I think? - but it doesn't help reasoning about it, nor does it make it correct)

Comment 24

[Brian Grinstead [:bgrins]]

> ::: browser/themes/windows/browser.css
> @@ +960,5 @@
> >    background-clip: padding-box !important;
> >    background-color: hsla(210,25%,98%,.08) !important;
> >    padding: 6px !important;
> >    border-style: none !important;
> > +  box-shadow: var(--toolbarbutton-hover-boxshadow);
> 
> This means that on the devtools theme, this will use the hover box-shadow
> all the time, right? Including when not hovered, and when hover:active?

(In reply to :Gijs Kruitbosch from comment #23)
> (fundamentally, it's kind of weird to have a variable named ...-hover-...
> that's used... all the time, not just on hover. I realize it needs to be the
> same name - I think? - but it doesn't help reasoning about it, nor does it
> make it correct)

Good points, maybe it would make more sense in this case to keep the same value hardcoded in two different places instead of reusing the variable.  Once when setting the variable (albeit in a kind of weird spot) and once for the actual non-hover state.  This is a case where the box shadow just happens to be the same in both cases, so we should probably keep them separate.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: win8-boxshadow.patch

I think this is what you were meaning

Comment 26

[:Gijs (he/him)]

Comment on attachment 8515430 [details] [diff] [review]
win8-boxshadow.patch

Looks good, thanks!

Comment 27

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/66cb6b48e415

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66cb6b48e415

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38c9453dec19