1363842 - Implement new toolbar button hover and active background styles

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1363842 - Implement new toolbar button hover and active background styles.

Review commit: https://reviewboard.mozilla.org/r/138138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138138/

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8866514 [details]
Bug 1363842 - Implement new toolbar button hover and active background styles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138138/diff/1-2/

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8866514 [details]
Bug 1363842 - Implement new toolbar button hover and active background styles.

https://reviewboard.mozilla.org/r/138138/#review141664

::: browser/themes/osx/browser.css:43
(Diff revision 2)
>    --toolbarbutton-hover-boxshadow: 0 1px 0 hsla(0,0%,100%,.5),
>                                     0 1px 0 hsla(0,0%,100%,.5) inset;
>  
> +%ifndef MOZ_PHOTON_THEME
>    --toolbarbutton-active-background: hsla(0,0%,0%,.02) linear-gradient(hsla(0,0%,0%,.12), transparent) border-box;
> +%endif

Please reorder the above lines such that you have this only once:

%ifdef MOZ_PHOTON_THEME
  ...
%else
  ...
%endif

::: browser/themes/osx/browser.css
(Diff revision 2)
> -  }
> -  #forward-button:-moz-window-inactive > .toolbarbutton-icon {
> -    box-shadow: 0 1px 0 0 rgba(0,0,0,0.2) inset,
> -                0 -1px 0 0 rgba(0,0,0,0.2) inset !important;
> -  }
> -}

We'll want to keep this in an %ifndef MOZ_PHOTON_THEME

::: browser/themes/shared/toolbarbuttons.inc.css:36
(Diff revision 2)
> +%endif
>    --toolbarbutton-hover-bordercolor: rgba(0,0,0,.2);
>  
> +%ifndef MOZ_PHOTON_THEME
>    --toolbarbutton-active-background: rgba(70%,70%,70%,.25);
> +%endif

See my comment on browser/themes/osx/browser.css

::: browser/themes/windows/browser.css:37
(Diff revision 2)
>    --toolbarbutton-hover-bordercolor: rgba(0,0,0,.2);
>    --toolbarbutton-hover-boxshadow: none;
>  
> +%ifndef MOZ_PHOTON_THEME
>    --toolbarbutton-active-background: rgba(0,0,0,.15);
> +%endif

ditto

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8866514 [details]
Bug 1363842 - Implement new toolbar button hover and active background styles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138138/diff/2-3/

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8866514 [details]
Bug 1363842 - Implement new toolbar button hover and active background styles.

https://reviewboard.mozilla.org/r/138138/#review143012

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/374e39d64190
Implement new toolbar button hover and active background styles. r=dao

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/374e39d64190

Comment 8

[ovidiu boca[:Ovidiu]]

Hi Nihanth,

Can you please help us here and tell us what we need to test? Thanks

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to ovidiu boca[:Ovidiu] from comment #8)
> Hi Nihanth,
> 
> Can you please help us here and tell us what we need to test? Thanks

The background of toolbar buttons when the mouse is moved over them and when they are clicked should match the spec (http://design.firefox.com/people/shorlander/photon/Mockups/macOS.html, this particular styling should be consistent across platforms)

Comment 10

[ovidiu boca[:Ovidiu]]

Hi Nihanth,

Thanks for your help. I retested and on Mac OS X 10.10 and 10.12 the actual results match the specification. On Windows, I have other colours for the same actions. When the mouse is over: #515152 and when the button is clicked: #656566 and the expected from the spec are, when the mouse is over: #E1E1E2 and when the button is clicked: #D5D5D6. We used xscope to determine the colour code. Please tell us your opinion. Thanks

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to ovidiu boca[:Ovidiu] from comment #10)
> Hi Nihanth,
> 
> Thanks for your help. I retested and on Mac OS X 10.10 and 10.12 the actual
> results match the specification. On Windows, I have other colours for the
> same actions. When the mouse is over: #515152 and when the button is
> clicked: #656566 and the expected from the spec are, when the mouse is over:
> #E1E1E2 and when the button is clicked: #D5D5D6. We used xscope to determine
> the colour code. Please tell us your opinion. Thanks

Interesting. As far as I know the toolbar background color is the same on Windows and MacOS, so I don't know why this is happening. I'll check on my Windows laptop later this evening.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

I double checked this and the color values for the background are identical on macOS and Windows: rgba(12, 12, 13, 0.1) for hover and rgba(12, 12, 13, 0.15) for active.

The toolbar background is the same too, so I'm not sure why the colours are different. Anyway, I don't think we will change this, because the colours are opacity-based and programmatically, they match the spec.

Comment 13

[ovidiu boca[:Ovidiu]]

I can confirm that the toolbar background is the same. The difference appears on the toolbar buttons when the mouse is over or when the buttons are clicked. But from what I see this bug deals the toolbar background, correct?

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to ovidiu boca[:Ovidiu] from comment #13)
> I can confirm that the toolbar background is the same. The difference
> appears on the toolbar buttons when the mouse is over or when the buttons
> are clicked. But from what I see this bug deals the toolbar background,
> correct?

No, this bug deals with the toolbar buttons, when the mouse is over or when they are clicked. I'll check again, but I think that code-wise, things are correct. I'm not sure why the actual computed colour might be varying (on my machines, it's fine). I'll check again and also bring it up in the photon-visual meeting today.

Comment 15

[Brindusa Tot, Desktop QA]

(In reply to Nihanth Subramanya [:nhnt11] from comment #14)
> No, this bug deals with the toolbar buttons, when the mouse is over or when
> they are clicked. I'll check again, but I think that code-wise, things are
> correct. I'm not sure why the actual computed colour might be varying (on my
> machines, it's fine). I'll check again and also bring it up in the
> photon-visual meeting today.

Nihanth, did you get a chance to look at this?

Comment 16

[ovidiu boca[:Ovidiu]]

I verified this on the latest Nightly 57.0a1(2017-09-14) and I can't reproduce it anymore. Based on this and comment 14 I will mark this as verified fixed.