1365195 - [Photon] Implement new back button appearance

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1365195 - [Photon] Implement new back button appearance.

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

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

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

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review143424

patching file browser/themes/shared/toolbarbuttons.inc.css
Hunk #1 FAILED at 18

Please rebase. Thanks!

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

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

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

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

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/4-5/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review143584

waiting for another rebase

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/5-6/

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/6-7/

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review144016

::: browser/themes/shared/toolbarbuttons.inc.css:19
(Diff revision 6)
>    --toolbarbutton-hover-background: hsla(240, 5%, 5%, .1);
>    --toolbarbutton-active-background: hsla(240, 5%, 5%, .15);
>  
>    --toolbarbutton-inner-padding: 5px;
> +
> +  --backbutton-background: hsla(0, 100%, 100%, .8);

This doesn't look right with dark OS and lightweight themes.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/7-8/

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review144050

::: browser/themes/shared/compacttheme.inc.css:200
(Diff revision 8)
>  /* Back and forward button */
>  
> +%ifdef MOZ_PHOTON_THEME
> +#back-button > .toolbarbutton-icon {
> +  border-radius: var(--toolbarbutton-border-radius) !important;
> +  padding: var(--toolbarbutton-inner-padding) !important;

Let's not add this. For photon, "compact" themes will just change colors, and we'll do the compact/touch switch independently from these two themes.

::: browser/themes/shared/toolbarbuttons.inc.css:394
(Diff revision 8)
> +%ifdef MOZ_PHOTON_THEME
> +#back-button:not(:active) > .toolbarbutton-icon {
> +  background: var(--backbutton-background) !important;
> +}
> +
> +#back-button:hover > .toolbarbutton-icon {

This incorrectly affects the disabled back button.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/8-9/

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/9-10/

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review144064

please see my previous review comments

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/10-11/

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/11-12/

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/12-13/

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review144118

::: browser/themes/shared/toolbarbuttons.inc.css:288
(Diff revision 13)
>  #nav-bar .toolbarbutton-1:not([disabled=true]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
> +%ifdef MOZ_PHOTON_THEME
> +#nav-bar :not(#back-button).toolbarbutton-1:not([disabled=true]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-icon,
> +%else
>  #nav-bar .toolbarbutton-1:not([disabled=true]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-icon,
> +%endif

We need this rule to cover the back button for compact mode. Better leave this as is and override it when needed.

::: browser/themes/shared/toolbarbuttons.inc.css:401
(Diff revision 13)
> +  box-shadow: var(--backbutton-hover-box-shadow);
> +  border-color: var(--backbutton-hover-border-color);
> +}
> +
> +#back-button:not([disabled]):hover:active > .toolbarbutton-icon {
> +  border-color: var(--backbutton-active-border-color);

Why are these things in CSS variables? You seem to be using them only once.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review144016

> This doesn't look right with dark OS and lightweight themes.

This is still an issue.

Comment 21

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/13-14/

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review145128

::: browser/themes/shared/toolbarbuttons.inc.css:47
(Diff revision 14)
>  
>    --toolbarbutton-checkedhover-backgroundcolor: rgba(85%,85%,85%,.25);
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +toolbar[brighttext]:-moz-lwtheme {

Why not just toolbar[brighttext]?

Comment 23

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review145130

::: browser/themes/shared/toolbarbuttons.inc.css:47
(Diff revision 14)
>  
>    --toolbarbutton-checkedhover-backgroundcolor: rgba(85%,85%,85%,.25);
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +toolbar[brighttext]:-moz-lwtheme {

Good point, thanks. I was attempting to compensate for the lwtheme version of backbutton-background that got ifdef'd out, and figured it was only needed for brighttext, but yeah, it applies to :not(-moz-lwtheme) as well.

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/14-15/

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

https://reviewboard.mozilla.org/r/139668/#review145134

::: browser/themes/shared/toolbarbuttons.inc.css:397
(Diff revision 15)
>  }
>  
> +%ifdef MOZ_PHOTON_THEME
> +#back-button:not([disabled]):hover > .toolbarbutton-icon {
> +  background: var(--backbutton-background) !important;
> +  box-shadow: 0 1px 6px hsla(0, 0%, 0%, .1);

nit: drop spaces inside hsla() (across the patch)

Comment 26

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139668/diff/15-16/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07facc83000c
[Photon] Implement new back button appearance. r=dao

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07facc83000c

Comment 29

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=f9ca97a334296facd2e0ea5582e7f12d0fe70fe4&newProject=mozilla-central&newRev=d712c82c59ec5a277047a75d09bec48be4a64b87

Comment 30

[ovidiu boca[:Ovidiu]]

Tested this issue on Windows 7 x32, Windows 8 x64, Windows 10 x64 and Ubuntu 16.04 x64 with FF Nightly 57.0a1(2017-08-08)and I can confirm the fix.