Closed Bug 1382033 Opened 6 years ago Closed 6 years ago

[Photon] Update styling of view button in history sidebar on macOS

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [photon-visual])

Attachments

(2 files)

Screenshot
200.85 KB, image/png
Details
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.
59 bytes, text/x-review-board-request
dao
: review+
Details
Attached image Screenshot — 
This is in the interest of styling the button consistently on all platforms.

Stephen, what do you think of the attached screenshot? Patch coming up.
Attachment #8887695 - Flags: feedback?(shorlander)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-visual]
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review164036

::: browser/themes/osx/places/places.css:126
(Diff revision 1)
>  #viewButton {
>    -moz-appearance: none;
> -  padding-bottom: 1px;
> -  padding-inline-start: 5px;
> -  padding-inline-end: 0px;
> +  font-size: 12px;
> +  border-radius: 4px;
> +  box-sizing: border-box;
> +  border: 1px solid transparent;

What's the point of this transparent border?

::: browser/themes/osx/places/places.css:137
(Diff revision 1)
> -#viewButton:focus {
> -  box-shadow: 0 1px 0 hsla(0, 0%, 0%, .15),
> +#viewButton[open] {
> +  background: hsla(240, 5%, 5%, 0.15);
> -              0 0 0 1px hsla(210, 100%, 60%, .45) inset,
> -              0 0 0 2px hsla(210, 100%, 60%, .45);
> -  border-color: hsla(210, 100%, 60%, 1);
>  }

We'll still need a focus ring for this, you can use var(--focus-ring-box-shadow).
Attachment #8887705 - Flags: review?(dao+bmo) → review-
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review164592

I missed a few things last time around...

::: browser/themes/osx/places/places.css:123
(Diff revision 2)
>    }
>  }
>  
>  #viewButton {
>    -moz-appearance: none;
> -  padding-bottom: 1px;
> +  font-size: 12px;

Is the default font too small or too large or what's going on here?

In any case, please use em rather than px.

::: browser/themes/osx/places/places.css:146
(Diff revision 2)
>  
>  #sidebar-search-container {
>    margin: 0 4px;
>  }
>  
> +#viewButton .button-menu-dropmarker {

Please use the child selector

::: browser/themes/osx/places/places.css:150
(Diff revision 2)
>  
> +#viewButton .button-menu-dropmarker {
> +  display: -moz-box;
> +  list-style-image: url("chrome://browser/skin/arrow-dropdown.svg");
> +  width: 12px;
> +  height: 12px;

Hmm, how does this work? arrow-dropdown.svg is 16*16 px.
Attachment #8887705 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #4)
> Comment on attachment 8887705 [details]
> Bug 1382033 - [Photon] Update styling of view button in history sidebar on
> macOS.
> 
> https://reviewboard.mozilla.org/r/158608/#review164592
> 
> I missed a few things last time around...
> 
> ::: browser/themes/osx/places/places.css:123
> (Diff revision 2)
> >    }
> >  }
> >  
> >  #viewButton {
> >    -moz-appearance: none;
> > -  padding-bottom: 1px;
> > +  font-size: 12px;
> 
> Is the default font too small or too large or what's going on here?
> 
> In any case, please use em rather than px.

The sidebar font size is set to 12px here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/places.css#23
I just copied this for consistency. I'll change both to ems.

> ::: browser/themes/osx/places/places.css:150
> (Diff revision 2)
> >  
> > +#viewButton .button-menu-dropmarker {
> > +  display: -moz-box;
> > +  list-style-image: url("chrome://browser/skin/arrow-dropdown.svg");
> > +  width: 12px;
> > +  height: 12px;
> 
> Hmm, how does this work? arrow-dropdown.svg is 16*16 px.

I was trying to mimic the sidebar switcher button; go the styling from https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/sidebar.inc.css#59.
(In reply to Nihanth Subramanya [:nhnt11] from comment #5)
> The sidebar font size is set to 12px here:
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/
> places.css#23
> I just copied this for consistency. I'll change both to ems.

This should probably be on the root node so that it applies to the search field, the button as well as the tree.
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review164598

::: browser/themes/osx/places/places.css:10
(Diff revisions 2 - 3)
>  %include ../shared.inc
>  
>  /* Sidebars */
>  
> +:root {
> +  /* Default font size within sidebar browser is 16px */

Where does this come from? Are we not using font: message-box; here from global.css?
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review165140

::: browser/themes/osx/places/places.css:9
(Diff revision 4)
>  
>  %include ../shared.inc
>  
>  /* Sidebars */
>  
> +:root > * {

Selector is too generic. places.css is used in many documents, not just sidebars.
Attachment #8887705 - Flags: review?(dao+bmo) → review-
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review175806

::: browser/themes/osx/places/places.css:126
(Diff revisions 4 - 5)
>  #viewButton {
>    -moz-appearance: none;
>    border-radius: 4px;
> -  box-sizing: border-box;
>    padding: 2px 4px;
> -  margin: 0;
> +  margin: 4px 0;

The searchbox has a 4px margin that, when removed, breaks the styling of the textbox. So I removed the 4px padding from the container and added a margin to the view button to match the textbox.
Flags: qe-verify? → qe-verify+
QA Contact: ovidiu.boca
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

https://reviewboard.mozilla.org/r/158608/#review177318

::: browser/themes/osx/places/places.css:5
(Diff revision 5)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %include ../shared.inc

Since you removed @roundButtonPressedShadow@, please remove this too.
Attachment #8887705 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2f9462c542b9
[Photon] Update styling of view button in history sidebar on macOS. r=dao
Attachment #8887695 - Flags: feedback?(shorlander)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4af061a811a5
Backed out changeset 2f9462c542b9 for failures in browser_all_files_referenced.js a=backout
Comment on attachment 8887705 [details]
Bug 1382033 - [Photon] Update styling of view button in history sidebar on macOS.

Updated the patch to reference the new location of arrow-dropdown-12.svg in toolkit/, and removed now unused menulist-dropmarker.png.
Flags: needinfo?(nhnt11)
Attachment #8887705 - Flags: review+ → review?(dao+bmo)
Attachment #8887705 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c12d1b399134
[Photon] Update styling of view button in history sidebar on macOS. r=dao
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
https://hg.mozilla.org/mozilla-central/rev/c12d1b399134
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Hi Nihanth,

Here we need to verify on all OSes if the styling of the buttons is the same as the one presented in the screenshot? 
Thanks
Flags: needinfo?(nhnt11)
(In reply to ovidiu boca[:Ovidiu] from comment #22)
> Hi Nihanth,
> 
> Here we need to verify on all OSes if the styling of the buttons is the same
> as the one presented in the screenshot? 
> Thanks

This bug only updated the button for MacOS.

On MacOS, the button should now match other Photon style buttons:
- Just the label and arrow icon when the menu is not open and the mouse is elsewhere
- Rounded-border background on hover
- Darker background when menu is open

Background colors for hover/active should match the sidebar close button (for example) although the exact color value might differ due to the sidebar background having vibrancy (the translucent/blurred effect on MacOS) enabled.
Flags: needinfo?(nhnt11)
Thanks Nihanth for your help, I verified this on Mac OS X 10.10 with the latest Nightly 57.0a1(2017-09-05) and I can confirm the fix.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.