Closed Bug 1367242 Opened 7 years ago Closed 7 years ago

[Photon] Implement new styling for header and search box of bookmarks/history sidebar.

Categories

(Firefox :: Theme, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Status
firefox56 --- verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

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

Attachments

(4 files)

      No description provided.
Summary: [Photon] Implement new styling for search box in history sidebar → [Photon] Implement new styling for search box in history/bookmarks sidebar
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Whiteboard: [photon-visual][p1][57] [triage] → [photon-visual][p1][57]
Summary: [Photon] Implement new styling for search box in history/bookmarks sidebar → [Photon] Implement new styling for header and search box of bookmarks/history sidebar.
I've included a short description of what each commit is doing as part of the extended commit message.
Also, my editor got rid of a bunch of trailing spaces in the touched files, let me know if you'd like me to revert those lines to preserve blame.
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Comment on attachment 8872487 [details]
Bug 1367242 - Part 1: Search bar should not be compact; should have a placeholder.

https://reviewboard.mozilla.org/r/143978/#review147832

::: browser/components/places/content/bookmarksPanel.xul:40
(Diff revision 2)
> +#ifndef MOZ_PHOTON_THEME
>      <textbox id="search-box" flex="1" type="search" class="compact"
> +#else
> +    <textbox id="search-box" flex="1" type="search"
> +             placeholder="&search.placeholder;"
> +#endif

Let's make these changes independently from MOZ_PHOTON_THEME.
Attachment #8872487 - Flags: review?(dao+bmo)
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review147834

::: browser/themes/shared/sidebar.inc.css:16
(Diff revision 2)
>  }
>  
>  .sidebar-header,
>  #sidebar-header {
> +%ifdef MOZ_PHOTON_THEME
> +  font-size: 16px;

No hardcoding of font sizes, see bug 1367009.

::: browser/themes/shared/sidebar.inc.css:19
(Diff revision 2)
>  #sidebar-header {
> +%ifdef MOZ_PHOTON_THEME
> +  font-size: 16px;
> +  font-weight: lighter;
> +  padding: 8px;
> +  border-bottom: 1px solid #bebebe;

Please use a system color, at least on Windows and Linux.
Attachment #8872489 - Flags: review?(dao+bmo) → review-
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1]
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Stephen, could you please clarify which spec is style-wise accurate between https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252085 and https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html?
Flags: needinfo?(shorlander)
We (Aaron, Stephen, and I) agreed on Slack to go with the larger font size for the sidebar switcher.
Flags: needinfo?(shorlander)
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review147834

> Please use a system color, at least on Windows and Linux.

Didn't use a system color, but changed it to match the spec, which I should have done in the first place but forgot. (https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html, the border colour is the same on all platforms).
Comment on attachment 8872487 [details]
Bug 1367242 - Part 1: Search bar should not be compact; should have a placeholder.

https://reviewboard.mozilla.org/r/143978/#review148722

Can you please update the commit messages to say what each individual patch is doing rather than [bug summary] - part x?

::: browser/components/places/content/bookmarksPanel.xul:36
(Diff revision 3)
>  
>    <hbox id="sidebar-search-container" align="center">
>      <label id="sidebar-search-label"
>             value="&search.label;" accesskey="&search.accesskey;" control="search-box"/>
> -    <textbox id="search-box" flex="1" type="search" class="compact"
> +    <textbox id="search-box" flex="1" type="search"
> +             placeholder="&search.placeholder;"

Looks like you're adding the placeholder while keeping the label? Isn't the placeholder supposed to replace the label?
Attachment #8872487 - Flags: review?(dao+bmo)
Comment on attachment 8872488 [details]
Bug 1367242 - Part 2: Reduce margin below search box, remove top border from tree view.

https://reviewboard.mozilla.org/r/143980/#review148728

::: browser/themes/osx/places/places.css:30
(Diff revision 3)
>  
>  .sidebar-placesTree {
>    -moz-appearance: -moz-mac-source-list;
>  }
>  
> +%ifndef MOZ_PHOTON_THEME

As mentioned earlier, we should do this independently from MOZ_PHOTON_THEME.
Attachment #8872488 - Flags: review?(dao+bmo)
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review148732

::: browser/themes/osx/browser.css:1355
(Diff revision 3)
>    -moz-appearance: -moz-mac-source-list;
>  }
>  
>  #sidebar-header {
> +%ifdef MOZ_PHOTON_THEME
> +  /* system font size is a bit smaller in mac, so need more ems. */

Do we? If fronts are generally smaller on Mac, wouldn't it make sense to have the header be smaller as well by the same ratio?

::: browser/themes/shared/sidebar.inc.css:18
(Diff revision 3)
>  #sidebar-header {
> +%ifdef MOZ_PHOTON_THEME
> +  font-size: 1.333em;
> +  font-weight: lighter;
> +  padding: 8px;
> +  border-bottom: 1px solid hsla(240, 5%, 5%, .1);

Please use ThreeDLightShadow on Windows and ThreeDShadow on Linux to make this border visible on dark OS themes.

::: browser/themes/shared/sidebar.inc.css:30
(Diff revision 3)
>  }
>  
>  .sidebar-splitter {
>    -moz-appearance: none;
> +%ifdef MOZ_PHOTON_THEME
> +  border: 0 solid hsla(240, 5%, 5%, .1);

ditto
Attachment #8872489 - Flags: review?(dao+bmo) → review-
Comment on attachment 8872490 [details]
Bug 1367242 - Part 4: Update styling of sidebar header elements.

https://reviewboard.mozilla.org/r/143984/#review148734

Again, please remove the MOZ_PHOTON_THEME ifdefs
Attachment #8872490 - Flags: review?(dao+bmo)
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review148732

> Do we? If fronts are generally smaller on Mac, wouldn't it make sense to have the header be smaller as well by the same ratio?

Well, the font in the sidebar is already larger than the system size (12px vs 11px) so I kept it relative to that.
Comment on attachment 8872487 [details]
Bug 1367242 - Part 1: Search bar should not be compact; should have a placeholder.

https://reviewboard.mozilla.org/r/143978/#review152848

::: commit-message-45411:1
(Diff revision 4)
> +Bug 1367242 - [Photon] Implement new styling for header and search box of bookmarks/history sidebar - part 1. r=dao

just drop the bug summary from the commit message:

Bug 1367242 - part 1: Make search field non-compact and replace label with placeholder string
Attachment #8872487 - Flags: review?(dao+bmo) → review+
Comment on attachment 8872488 [details]
Bug 1367242 - Part 2: Reduce margin below search box, remove top border from tree view.

https://reviewboard.mozilla.org/r/143980/#review152854

::: commit-message-62a4b:1
(Diff revision 4)
> +Bug 1367242 - [Photon] Implement new styling for header and search box of bookmarks/history sidebar - part 2. r=dao

Bug 1367242 - part 2: Reduce margin below search box, remove top border from tree view
Attachment #8872488 - Flags: review?(dao+bmo) → review+
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review152862

::: commit-message-505b5:1
(Diff revision 5)
> +Bug 1367242 - [Photon] Implement new styling for header and search box of bookmarks/history sidebar - part 3. r=dao

Bug 1367242 - part 3: Increase padding and font size of sidebar header, add bottom border

::: browser/themes/shared/sidebar.inc.css:24
(Diff revision 5)
>    text-shadow: none;
>  }
>  
>  .sidebar-splitter {
>    -moz-appearance: none;
> -  border: 0 solid #ccc;
> +  border: 0 solid hsla(240, 5%, 5%, .1);

please move the border color to the mac stylesheet
Attachment #8872489 - Flags: review?(dao+bmo)
Comment on attachment 8872490 [details]
Bug 1367242 - Part 4: Update styling of sidebar header elements.

https://reviewboard.mozilla.org/r/143984/#review152866

::: browser/themes/shared/sidebar.inc.css:9
(Diff revision 7)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  %endif
>  
>  #sidebar-box {
> -  --header-background-color: #F2F2F2;
> -  --header-background-color-hover: rgba(204, 204, 204, 0.6);
> +  --header-background-color-hover: hsla(240, 5%, 5%, 0.05);
> +  --header-background-color-active: hsla(240, 5%, 5%, 0.1);

Let's get rid of these variables.

::: browser/themes/shared/sidebar.inc.css:83
(Diff revision 7)
>  }
>  
>  #sidebar-switcher-target {
>    -moz-appearance: none;
> -  padding: 4px;
> -  color: inherit;
> +  margin-inline-end: 4px;
> +  border-radius: 4px;

Why did you remove color: inherit?

::: browser/themes/shared/sidebar.inc.css:93
(Diff revision 7)
> -#sidebar-switcher-target.active,
>  #sidebar-close:hover {
>    background: var(--header-background-color-hover);
>  }
>  
> +#sidebar-switcher-target:hover{

space before {

::: browser/themes/shared/sidebar.inc.css:104
(Diff revision 7)
> +#sidebar-switcher-target.active {
> +  background: var(--header-background-color-active);
> +}
> +
> +#sidebar-switcher-target:hover:active,
> +#sidebar-switcher-target.active {

I don't understand why there are two rules for #sidebar-switcher-target:hover:active and #sidebar-switcher-target.active.
Attachment #8872490 - Flags: review?(dao+bmo) → review-
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment on attachment 8872490 [details]
Bug 1367242 - Part 4: Update styling of sidebar header elements.

https://reviewboard.mozilla.org/r/143984/#review152866

> Why did you remove color: inherit?

Ugh, probably a rebase mistake because I'm clumsy.

> I don't understand why there are two rules for #sidebar-switcher-target:hover:active and #sidebar-switcher-target.active.

So, the .active rule is for how the button should be styled while the menu is open; i.e. we still need a :hover:active rule for when the mouse is being held down on the button but the menu hasn't been shown yet. We need the rule for .active for when the menu is open and the mouse button has been lifted.
Blocks: 1367149
Comment on attachment 8872489 [details]
Bug 1367242 - Part 3: Increase padding and font size of sidebar header, add bottom border.

https://reviewboard.mozilla.org/r/143982/#review152914
Attachment #8872489 - Flags: review?(dao+bmo) → review+
Comment on attachment 8872490 [details]
Bug 1367242 - Part 4: Update styling of sidebar header elements.

https://reviewboard.mozilla.org/r/143984/#review152916
Attachment #8872490 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/af0a7f4c0966
Part 1: Search bar should not be compact; should have a placeholder. r=dao
https://hg.mozilla.org/integration/autoland/rev/62629d511c74
Part 2: Reduce margin below search box, remove top border from tree view. r=dao
https://hg.mozilla.org/integration/autoland/rev/a3484b466856
Part 3: Increase padding and font size of sidebar header, add bottom border. r=dao
https://hg.mozilla.org/integration/autoland/rev/0ddb77735de4
Part 4: Update styling of sidebar header elements. r=dao
I have reproduced this bug with Nightly 55.0a1 (2017-05-23) on Windows 7, 64-bit.

The bug's fix is now verified on Latest Nightly 56.0a1

Build ID 	20170615030208

User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
  
[bugday-20170614]
I have reproduced this bug with Nightly 55.0a1 (2017-05-23) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170615100219
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170614]
As per comment 46 & comment 47, I am marking this bug's status as verified fixed!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1374812
Depends on: 1376109
No longer blocks: 1377003
Depends on: 1377825
Depends on: 1481091
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: