1125677 - "find on page" tab has hard-to-read text in dev edition theme

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: Screen Shot 2015-01-26 at 12.04.52 AM.png

In dark mode with the dev edition theme, the find on page bar (apple+F) is styled like the rest of the chrome, which looks solid, except some of the text is too dark to read well.

Comment 1

[Brian Grinstead [:bgrins]]

This is not a problem in 36.  Tim, do you remember what bug would be affecting the find bar background / text color in 37+?

Comment 2

[Tim Nguyen :ntim]

Bug 891258 regressed this. For this bug, we can just override [0] inside devedition.css


[0] : http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/findBar.css#139

Comment 3

[Tim Nguyen :ntim]

Related bug : bug 1121432

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: findbar-text-styling.patch

Updates the text style on findbar to just inherit so it looks reasonable on light and dark theme.  Also updates the ::-moz-selection styling for the textbox since it's color is updated.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8556540 [details] [diff] [review]
findbar-text-styling.patch

Forgot to flag for review

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8556540 [details] [diff] [review]
findbar-text-styling.patch

Review of attachment 8556540 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/devedition.inc.css
@@ +187,5 @@
>    background-image: none;
>  }
>  
> +/* Default findbar text color doesn't look good - Bug 1125677 */
> +.findbar-find-status,

Do we need to prefix this with something like .browserContainer > findbar to make sure it's only targeting this particular element?  The styles being overriden here are in toolkit/themes/*/global/findBar.css.

Comment 7

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8556540 [details] [diff] [review]
> findbar-text-styling.patch
> 
> Review of attachment 8556540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devedition.inc.css
> @@ +187,5 @@
> >    background-image: none;
> >  }
> >  
> > +/* Default findbar text color doesn't look good - Bug 1125677 */
> > +.findbar-find-status,
> 
> Do we need to prefix this with something like .browserContainer > findbar to
> make sure it's only targeting this particular element?  The styles being
> overriden here are in toolkit/themes/*/global/findBar.css.

Maybe... what happens in e.g. view source right now? OTOH, in what other kinds of windows is devedition.css loaded?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > Comment on attachment 8556540 [details] [diff] [review]
> > findbar-text-styling.patch
> > 
> > Review of attachment 8556540 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +187,5 @@
> > >    background-image: none;
> > >  }
> > >  
> > > +/* Default findbar text color doesn't look good - Bug 1125677 */
> > > +.findbar-find-status,
> > 
> > Do we need to prefix this with something like .browserContainer > findbar to
> > make sure it's only targeting this particular element?  The styles being
> > overriden here are in toolkit/themes/*/global/findBar.css.
> 
> Maybe... what happens in e.g. view source right now? OTOH, in what other
> kinds of windows is devedition.css loaded?

Just confirmed - it doesn't get applied to the view source window.  Devedition.css is only loaded in browser.xul.  Although I guess an extension could inject a findbar element inside of this window?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8556540 [details] [diff] [review]
findbar-text-styling.patch

Review of attachment 8556540 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with, yeah, better-safe-than-sorry specific selector.

::: browser/themes/shared/devedition.inc.css
@@ +187,5 @@
>    background-image: none;
>  }
>  
> +/* Default findbar text color doesn't look good - Bug 1125677 */
> +.findbar-find-status,

Probably wise in the end, even if there isn't anything in default Firefox, it doesn't really hurt, ditto for an equally not super-specific selector like ".found-matches" below. :-)

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: findbar-text-styling-r=Gijs.patch

With better safe than sorry selector

Comment 11

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/c95bb70fac98

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c95bb70fac98