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)

Product:
Toolkit
Component:
Themes
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(2 files)

Bug 1455922 - Create shared findbar CSS across platforms.
59 bytes, text/x-review-board-request
dao
: review+
ryanvm
: approval-mozilla-beta+
Details
Bug 1455922 - Adjust findbar style to work better on dark themes.
59 bytes, text/x-review-board-request
dao
: review+
ryanvm
: approval-mozilla-beta+
Details
Component: Developer Tools → Themes
Product: Firefox → Toolkit
Summary: Adjust findbar buttons to work better on dark themes → Adjust findbar style to work better on dark themes
Assignee: nobody → ntim.bugs
Attachment #8969994 - Flags: review?(jaws) → review?(mdeboer)
Attachment #8970000 - Flags: review?(jaws) → review?(mdeboer)
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 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 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)
(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.
Attachment #8969994 - Flags: review?(mdeboer) → review?(dao+bmo)
Attachment #8970000 - Flags: review?(mdeboer) → review?(dao+bmo)
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 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 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)
Priority: -- → P3
Attachment #8970000 - Flags: review?(dao+bmo)
(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)
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
(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)
Attachment #8969994 - Flags: review?(dao+bmo)
Attachment #8970000 - Flags: review?(dao+bmo)
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 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+
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
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
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?
Comment on attachment 8969994 [details]
Bug 1455922 - Create shared findbar CSS across platforms.

See previous comment
Attachment #8969994 - Flags: approval-mozilla-beta?
Flags: qe-verify+
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+
Attachment #8970000 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
status-firefox62: fixed → verified
Verified fixed on Windows 10x64, Ubuntu 17.04 x64 and Mac OS X 10.13 using the latest Firefox Beta 61.0b12.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.