[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
a month ago
8 hours 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

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

Updated

a month ago
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]

Updated

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

Updated

26 days 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

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

Comment 10

26 days 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

25 days ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12

Comment 11

25 days 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

25 days 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

25 days ago
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1]

Updated

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

Comment 13

25 days 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

24 days 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

24 days 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

24 days 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

24 days 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

24 days 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

24 days 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

12 days 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

12 days 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

12 days 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

12 days 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

12 days 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

11 days 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

11 days 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

11 days ago
Blocks: 1367149

Comment 42

11 days 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

11 days 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

11 days 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

11 days 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: 11 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 46

9 days 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

9 days 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

9 days ago
As per comment 46 & comment 47, I am marking this bug's status as verified fixed!
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+

Updated

4 days ago
Depends on: 1374812

Updated

8 hours ago
Depends on: 1376109
You need to log in before you can comment on or make changes to this bug.