Closed Bug 1125677 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, defect)

37 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: jsantell, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [devedition-polish])

Attachments

(2 files, 1 obsolete file)

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.
This is not a problem in 36.  Tim, do you remember what bug would be affecting the find bar background / text color in 37+?
Flags: needinfo?(ntim007)
Keywords: regression
Whiteboard: [devedition-polish]
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
Blocks: 891258
Flags: needinfo?(ntim007)
Related bug : bug 1121432
See Also: → 1121432
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 1127351
Attached patch findbar-text-styling.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8556540 [details] [diff] [review]
findbar-text-styling.patch

Forgot to flag for review
Attachment #8556540 - Flags: review?(gijskruitbosch+bugs)
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.
(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?
Flags: needinfo?(bgrinstead)
(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?
Flags: needinfo?(bgrinstead)
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. :-)
Attachment #8556540 - Flags: review?(gijskruitbosch+bugs) → review+
With better safe than sorry selector
Attachment #8556540 - Attachment is obsolete: true
Attachment #8557137 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/c95bb70fac98
Whiteboard: [devedition-polish] → [fixed-in-fx-team][devedition-polish]
https://hg.mozilla.org/mozilla-central/rev/c95bb70fac98
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][devedition-polish] → [devedition-polish]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.