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 on attachment 8868065 [details] Bug 1365195 - [Photon] Implement new back button appearance. https://reviewboard.mozilla.org/r/139668/#review143584 waiting for another rebase

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

Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/07facc83000c [Photon] Implement new back button appearance. r=dao

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

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.