Closed
Bug 1455922
Opened 4 years ago
Closed 4 years ago
Adjust findbar style to work better on dark themes
Categories
(Toolkit :: Themes, defect, P3)
Toolkit
Themes
Tracking
()
VERIFIED
FIXED
mozilla62
People
(Reporter: ntim, Assigned: ntim)
References
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
dao
:
review+
ryanvm
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
dao
:
review+
ryanvm
:
approval-mozilla-beta+
|
Details |
Assignee | ||
Updated•4 years ago
|
Component: Developer Tools → Themes
Product: Firefox → Toolkit
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Summary: Adjust findbar buttons to work better on dark themes → Adjust findbar style to work better on dark themes
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → ntim.bugs
Assignee | ||
Updated•4 years ago
|
Attachment #8969994 -
Flags: review?(jaws) → review?(mdeboer)
Attachment #8970000 -
Flags: review?(jaws) → review?(mdeboer)
Assignee | ||
Comment 4•4 years ago
|
||
mozreview-review |
Comment on attachment 8970000 [details] Bug 1455922 - Adjust findbar style to work better on dark themes. https://reviewboard.mozilla.org/r/238770/#review244866 ::: toolkit/themes/shared/icons/find-next-arrow.svg:5 (Diff revision 1) > <!-- 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/. --> > <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > - <path fill="context-fill" d="M8 12a1 1 0 0 1-.707-.293l-5-5a1 1 0 0 1 1.414-1.414L8 9.586l4.293-4.293a1 1 0 0 1 1.414 1.414l-5 5A1 1 0 0 1 8 12z"/> > + <path fill="context-fill" fill-opacity="context-fill-opacity" d="M8 12a1 1 0 0 1-.707-.293l-5-5a1 1 0 0 1 1.414-1.414L8 9.586l4.293-4.293a1 1 0 0 1 1.414 1.414l-5 5A1 1 0 0 1 8 12z"/> note to self: this change isn't needed. ::: toolkit/themes/shared/icons/find-previous-arrow.svg:5 (Diff revision 1) > <!-- 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/. --> > <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > - <path fill="context-fill" d="M13 11a1 1 0 0 1-.707-.293L8 6.414l-4.293 4.293a1 1 0 0 1-1.414-1.414l5-5a1 1 0 0 1 1.414 0l5 5A1 1 0 0 1 13 11z"/> > + <path fill="context-fill" fill-opacity="context-fill-opacity" d="M13 11a1 1 0 0 1-.707-.293L8 6.414l-4.293 4.293a1 1 0 0 1-1.414-1.414l5-5a1 1 0 0 1 1.414 0l5 5A1 1 0 0 1 13 11z"/> note-to-self: ditto
Comment 5•4 years ago
|
||
mozreview-review |
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. https://reviewboard.mozilla.org/r/238768/#review246352 It looks like the new findBar.inc.css file is more a copy of the windows theme findBar.css than the linux theme one, which coïncidentally means that it's a bit hard to review this version. I'd like to take a look at the next version!
Attachment #8969994 -
Flags: review?(mdeboer)
Comment 6•4 years ago
|
||
mozreview-review |
Comment on attachment 8970000 [details] Bug 1455922 - Adjust findbar style to work better on dark themes. https://reviewboard.mozilla.org/r/238770/#review246356 Once part 1 is done, this'll be a r=me. Thanks for working on this! ::: toolkit/themes/shared/findBar.inc.css:78 (Diff revision 1) > border-color: Highlight; > } > > .findbar-textbox[status="notfound"] { > - background-color: #f66; > - color: white; > + background-color: rgba(255, 0, 57, 0.3); > + color: inherit; Is this necessary? As in, can't you simply remove the 'color' rule? Nit: please write as `rgba(255,0,57,.3)` (no spaces and leading 0 necessary). ::: toolkit/themes/shared/icons/find-next-arrow.svg:5 (Diff revision 1) > <!-- 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/. --> > <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > - <path fill="context-fill" d="M8 12a1 1 0 0 1-.707-.293l-5-5a1 1 0 0 1 1.414-1.414L8 9.586l4.293-4.293a1 1 0 0 1 1.414 1.414l-5 5A1 1 0 0 1 8 12z"/> > + <path fill="context-fill" fill-opacity="context-fill-opacity" d="M8 12a1 1 0 0 1-.707-.293l-5-5a1 1 0 0 1 1.414-1.414L8 9.586l4.293-4.293a1 1 0 0 1 1.414 1.414l-5 5A1 1 0 0 1 8 12z"/> Well, you did the same for the OSX looking glass one, but I think your change is actually for the better :). However, less code is (almost) always better, so I'm fine with no fille and context-fill properties. If they are needed after all, but their value doesn't matter, please keep you changes.
Attachment #8970000 -
Flags: review?(mdeboer)
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #6) > Comment on attachment 8970000 [details] > Bug 1455922 - Adjust findbar style to work better on dark themes. > > https://reviewboard.mozilla.org/r/238770/#review246356 > > Once part 1 is done, this'll be a r=me. Thanks for working on this! > > ::: toolkit/themes/shared/findBar.inc.css:78 > (Diff revision 1) > > border-color: Highlight; > > } > > > > .findbar-textbox[status="notfound"] { > > - background-color: #f66; > > - color: white; > > + background-color: rgba(255, 0, 57, 0.3); > > + color: inherit; > > Is this necessary? As in, can't you simply remove the 'color' rule? > Nit: please write as `rgba(255,0,57,.3)` (no spaces and leading 0 necessary). XUL Textboxes get color: -moz-fieldText by default.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•4 years ago
|
Attachment #8969994 -
Flags: review?(mdeboer) → review?(dao+bmo)
Attachment #8970000 -
Flags: review?(mdeboer) → review?(dao+bmo)
Comment 13•4 years ago
|
||
Sorry for the delay here, Tim, I just realized that I'm not good at reviewing this kind of large CSS changes. Dão reviewed my style changes to the findbar a couple of years ago and was very, very good at spotting issues with the refactor I did there. I trust him more with this than I do myself.
Comment 14•4 years ago
|
||
mozreview-review |
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. https://reviewboard.mozilla.org/r/238768/#review249316 ::: toolkit/themes/shared/findBar.inc.css (Diff revision 4) > } > > .findbar-closebutton { > - margin-inline-start: 4px; > + margin: 0 8px; > - padding-inline-start: 0; > - padding-inline-end: 8px; Why this change? The margin and padding are set up this way so that the button is clickable on the edge of the window.
Attachment #8969994 -
Flags: review?(dao+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•4 years ago
|
||
mozreview-review |
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. https://reviewboard.mozilla.org/r/238768/#review250642 ::: toolkit/themes/shared/findBar.inc.css:53 (Diff revision 5) > background-color: -moz-Field; > - border: 1px solid; > - border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > + border: 1px solid var(--lwt-toolbar-field-border-color, ThreeDShadow); > + border-radius: 2px; > - border-radius: 2px 0 0 2px; > margin: 0; > - padding: 1px 5px; > + padding: 2px 5px; On Ubuntu, this makes the textbox shorter than the Highlight All / Match Case / Whole Words buttons. We probably want it to keep the same height as the buttons?
Attachment #8969994 -
Flags: review?(dao+bmo)
Updated•4 years ago
|
Priority: -- → P3
Updated•4 years ago
|
Attachment #8970000 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 18•4 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #17) > Comment on attachment 8969994 [details] > Bug 1455922 - Create shared findbar CSS across platforms. > > https://reviewboard.mozilla.org/r/238768/#review250642 > > ::: toolkit/themes/shared/findBar.inc.css:53 > (Diff revision 5) > > background-color: -moz-Field; > > - border: 1px solid; > > - border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > > + border: 1px solid var(--lwt-toolbar-field-border-color, ThreeDShadow); > > + border-radius: 2px; > > - border-radius: 2px 0 0 2px; > > margin: 0; > > - padding: 1px 5px; > > + padding: 2px 5px; > > On Ubuntu, this makes the textbox shorter than the Highlight All / Match > Case / Whole Words buttons. We probably want it to keep the same height as > the buttons? I just checked, and it looks like it this was already the case on Windows/Mac. Also, just like on the other two platforms, the height of the Highlight All/Match Case/Whole Words buttons depends on the UI density.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 19•4 years ago
|
||
mozreview-review |
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. https://reviewboard.mozilla.org/r/238768/#review253168 ::: toolkit/themes/shared/findBar.inc.css:159 (Diff revision 5) > .findbar-entire-word > .toolbarbutton-icon { > display: none; > } > > .findbar-find-status, > -.found-matches { > +.findbar-matches { self note: Change this back to found-matches
Comment 20•4 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #18) > (In reply to Dão Gottwald [::dao] from comment #17) > > Comment on attachment 8969994 [details] > > Bug 1455922 - Create shared findbar CSS across platforms. > > > > https://reviewboard.mozilla.org/r/238768/#review250642 > > > > ::: toolkit/themes/shared/findBar.inc.css:53 > > (Diff revision 5) > > > background-color: -moz-Field; > > > - border: 1px solid; > > > - border-color: var(--lwt-toolbar-field-border-color, ThreeDShadow); > > > + border: 1px solid var(--lwt-toolbar-field-border-color, ThreeDShadow); > > > + border-radius: 2px; > > > - border-radius: 2px 0 0 2px; > > > margin: 0; > > > - padding: 1px 5px; > > > + padding: 2px 5px; > > > > On Ubuntu, this makes the textbox shorter than the Highlight All / Match > > Case / Whole Words buttons. We probably want it to keep the same height as > > the buttons? > > I just checked, and it looks like it this was already the case on > Windows/Mac. > > Also, just like on the other two platforms, the height of the Highlight > All/Match Case/Whole Words buttons depends on the UI density. Alright.
Flags: needinfo?(dao+bmo)
Assignee | ||
Updated•4 years ago
|
Attachment #8969994 -
Flags: review?(dao+bmo)
Assignee | ||
Updated•4 years ago
|
Attachment #8970000 -
Flags: review?(dao+bmo)
Comment 21•4 years ago
|
||
mozreview-review |
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. https://reviewboard.mozilla.org/r/238768/#review253482
Attachment #8969994 -
Flags: review?(dao+bmo) → review+
Comment 22•4 years ago
|
||
mozreview-review |
Comment on attachment 8970000 [details] Bug 1455922 - Adjust findbar style to work better on dark themes. https://reviewboard.mozilla.org/r/238770/#review253484
Attachment #8970000 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•4 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e57729f03ecb Create shared findbar CSS across platforms. r=dao https://hg.mozilla.org/integration/autoland/rev/51db5a12a410 Adjust findbar style to work better on dark themes. r=dao
Comment 26•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e57729f03ecb https://hg.mozilla.org/mozilla-central/rev/51db5a12a410
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 27•4 years ago
|
||
Comment on attachment 8970000 [details] Bug 1455922 - Adjust findbar style to work better on dark themes. Approval Request Comment [Feature/Bug causing the regression]: dark theme darkening [User impact if declined]: bad visibility of findbar arrows on dark theme [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: [Needs manual test from QE? If yes, steps to reproduce]: see screenshot [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: no [Why is the change risky/not risky?]: css only [String changes made/needed]: n/a
Attachment #8970000 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 28•4 years ago
|
||
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. See previous comment
Attachment #8969994 -
Flags: approval-mozilla-beta?
Updated•4 years ago
|
Flags: qe-verify+
Comment 29•4 years ago
|
||
Comment on attachment 8969994 [details] Bug 1455922 - Create shared findbar CSS across platforms. Low-risk polish fix for the dark theme work shipping in Fx61. Approved for 61.0b12.
Attachment #8969994 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•4 years ago
|
Attachment #8970000 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 30•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1bd083b09db8 https://hg.mozilla.org/releases/mozilla-beta/rev/a9bd7aeb9874
status-firefox61:
--- → fixed
Comment 31•4 years ago
|
||
Managed to reproduce the issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12 using Firefox Nightly 61.0a1 (2018-04-23), Firefox Nightly 62.0a1(2018-05-25) and Firefox Beta 61.0b10. Verified fixed on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12 using the latest Nightly 62.0a1(2018-06-06.
Comment 32•4 years ago
|
||
Verified fixed on Windows 10x64, Ubuntu 17.04 x64 and Mac OS X 10.13 using the latest Firefox Beta 61.0b12.
You need to log in
before you can comment on or make changes to this bug.
Description
•