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)
Firefox
Theme
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | verified |
People
(Reporter: nhnt11, Assigned: nhnt11)
References
Details
(Whiteboard: [photon-visual][p1])
Attachments
(4 files)
No description provided.
Assignee | ||
Updated•7 years ago
|
Summary: [Photon] Implement new styling for search box in history sidebar → [Photon] Implement new styling for search box in history/bookmarks sidebar
Updated•7 years ago
|
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1][57] [triage]
Updated•7 years ago
|
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Whiteboard: [photon-visual][p1][57] [triage] → [photon-visual][p1][57]
Assignee | ||
Updated•7 years 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•7 years ago
|
||
I've included a short description of what each commit is doing as part of the extended commit message.
Assignee | ||
Comment 10•7 years 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•7 years ago
|
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Comment 11•7 years 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•7 years 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•7 years ago
|
Whiteboard: [photon-visual][p1][57] → [photon-visual][p1]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
Assignee | ||
Comment 13•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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.
Comment 42•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 46•7 years 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•7 years 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•7 years ago
|
||
As per comment 46 & comment 47, I am marking this bug's status as verified fixed!
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•