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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Updated

3 months ago
Summary: [Photon] Implement new styling for search box in history sidebar → [Photon] Implement new styling for search box in history/bookmarks sidebar

Updated

3 months ago
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]

Updated

3 months ago
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Whiteboard: [photon-visual][p1][57] [triage] → [photon-visual][p1][57]
(Assignee)

Updated

3 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
I've included a short description of what each commit is doing as part of the extended commit message.
(Assignee)

Comment 10

3 months ago
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.

Updated

3 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12

Comment 11

3 months ago
mozreview-review
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 12

3 months ago
mozreview-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/#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-

Updated

3 months ago
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1]

Updated

3 months ago
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
(Assignee)

Comment 13

3 months ago
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)
(Assignee)

Comment 14

3 months ago
We (Aaron, Stephen, and I) agreed on Slack to go with the larger font size for the sidebar switcher.
Flags: needinfo?(shorlander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 21

3 months ago
mozreview-review
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 22

3 months ago
mozreview-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/#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 23

3 months ago
mozreview-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/#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 24

3 months ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

2 months ago
mozreview-review
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 34

2 months ago
mozreview-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 35

2 months ago
mozreview-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 36

2 months ago
mozreview-review
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-

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

2 months ago
mozreview-review-reply
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.

Updated

2 months ago
Blocks: 1367149

Comment 42

2 months ago
mozreview-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/#review152914
Attachment #8872489 - Flags: review?(dao+bmo) → review+

Comment 43

2 months ago
mozreview-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+

Comment 44

2 months ago
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

Comment 45

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af0a7f4c0966
https://hg.mozilla.org/mozilla-central/rev/62629d511c74
https://hg.mozilla.org/mozilla-central/rev/a3484b466856
https://hg.mozilla.org/mozilla-central/rev/0ddb77735de4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 46

2 months ago
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]

Comment 47

2 months ago
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]

Comment 48

2 months ago
As per comment 46 & comment 47, I am marking this bug's status as verified fixed!
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Updated

2 months ago
Flags: qe-verify+

Updated

2 months ago
Depends on: 1374812

Updated

2 months ago
Depends on: 1376109
(Assignee)

Updated

2 months ago
Blocks: 1377003
(Assignee)

Updated

2 months ago
No longer blocks: 1377003

Updated

2 months ago
Depends on: 1377825
You need to log in before you can comment on or make changes to this bug.