Closed Bug 1365195 Opened 4 years ago Closed 4 years ago

[Photon] Implement new back button appearance

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox57 --- verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(1 file)

Bug 1365195 - [Photon] Implement new back button appearance.
59 bytes, text/x-review-board-request
dao
: review+
Details 
No description provided.
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
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!
Attachment #8868065 - Flags: review?(dao+bmo)
Comment on attachment 8868065 [details]
Bug 1365195 - [Photon] Implement new back button appearance.

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

waiting for another rebase
Attachment #8868065 - Flags: review?(dao+bmo)
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.
Attachment #8868065 - Flags: review?(dao+bmo) → review-
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.
Attachment #8868065 - Flags: review?(dao+bmo) → review-
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
Attachment #8868065 - Flags: review?(dao+bmo) → review-
Blocks: 1365901
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.
Attachment #8868065 - Flags: review?(dao+bmo) → review-
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)
Attachment #8868065 - Flags: review?(dao+bmo) → review+
Blocks: 1366769
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07facc83000c
[Photon] Implement new back button appearance. r=dao
https://hg.mozilla.org/mozilla-central/rev/07facc83000c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1367273
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.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.