Closed Bug 1012629 Opened 10 years ago Closed 10 years ago

Infer from each toolbar's text color whether we should use inverted icons

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: access, Whiteboard: p=8 s=it-32c-31a-30b.2 [qa!])

Attachments

(2 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
* makes us use white icons on dark high-contrast themes (no bug on file, I think)

* fixes bug 947356 / bug 994623

* should work automatically for a dark private browsing theme when that's implemented

* it's a net code simplification (removes a bunch of rules on Windows in particular)
Attachment #8424774 - Flags: review?(gijskruitbosch+bugs)
Flags: firefox-backlog+
Comment on attachment 8424774 [details] [diff] [review]
patch

>+    function parseRGB(aColorString) {
>+      let rgb = aColorString.match(/^rgba?\((\d+), (\d+), (\d+)/);
>+      rgb.shift();
>+      return rgb.map(function (x) parseInt(x));
>+    }
>+
>+    for (let toolbar of toolbars) {
>+      let [r, g, b] = parseRGB(getComputedStyle(toolbar).color);
>+      let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
>+      if (luminance <= 110)
>+        toolbar.removeAttribute("brighttext");
>+      else
>+        toolbar.setAttribute("brighttext", "true");
>+    }

For the record, this is taken from http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/toolkit/modules/LightweightThemeConsumer.jsm#l115
Blocks: themea11y
Added to Iteration 32.2.
Whiteboard: p=8 → p=8 s=it-32c-31a-30b.2 [qa?]
Whiteboard: p=8 s=it-32c-31a-30b.2 [qa?] → p=8 s=it-32c-31a-30b.2 [qa+]
Comment on attachment 8424774 [details] [diff] [review]
patch

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

I have a bit too many questions, so I'm clearing review for now.

General ones:
- why leave out linux? Just for scope reasons or is there some other reason? Ideally, I'd like the Windows and Linux styles to be/remain as similar as reasonably possible. We certainly already invert icons there for lwthemes, so presumably that CSS should also be updated.
- can you try this against our talos suite? The sync reflows give me the creeps, and I suspect this will affect tpaint/ts_paint (although you'll need a bundle of retriggers to tell)

::: browser/base/content/browser-fullScreen.js
@@ +566,5 @@
>        navbar.appendChild(fullscreenctls);
>      }
>      fullscreenctls.hidden = aShow;
> +
> +    ToolbarIconColor.inferFromText();

Can't we listen for an event for the fullscreen case?

::: browser/base/content/browser.js
@@ +4545,5 @@
>        titlebar.style.marginBottom = "";
>        menubar.style.paddingBottom = "";
>      }
> +
> +    ToolbarIconColor.inferFromText();

Is there a way we can avoid this hitting all the toolbars all the time? Presumably, when tabsintitlebar gets turned off/on, only the tabstoolbar and toolbar-menubar (on non-OSX) colors are affected, if at all?

@@ +7215,5 @@
> +
> +  handleEvent: function (event) {
> +    switch (event.type) {
> +      case "activate":
> +      case "deactivate":

Can we build a lightweight caching solution for the active/inactive cases? I'm worried about doing sync reflows every time a window gains/loses focus.

Also, have you verified this doesn't duplicate the call you make from onLoad? I would expect an activate event on every new window, but perhaps that's naive (or it fires before onload?)

@@ +7224,5 @@
> +
> +  observe: function (aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case "lightweight-theme-styling-update":
> +        setTimeout(() => { this.inferFromText(); }, 0);

Why is this in a timeout?

@@ +7237,5 @@
> +#else
> +      document.querySelectorAll("#navigator-toolbox > toolbar");
> +#endif
> +
> +    function parseRGB(aColorString) {

If this + the luminance calculations are already somewhere else, can we refactor into a toolkit JSM? I expect devtools might also have similar code. Should probably followup that to avoid scope creep here though.

@@ +7248,5 @@
> +      let [r, g, b] = parseRGB(getComputedStyle(toolbar).color);
> +      let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> +      if (luminance <= 110)
> +        toolbar.removeAttribute("brighttext");
> +      else

Nit: guard this (!hasAttribute) because setAttributes aren't free.

::: browser/themes/linux/downloads/indicator.css
@@ +25,5 @@
>    min-width: 18px;
>    min-height: 18px;
>  }
>  
> +toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention]) > #downloads-indicator-anchor > #downloads-indicator-icon {

Here, and in several other places related to the download button, you're changing the selector, either by adding not clauses, or by changing to/from descendant selectors vs. child selectors. Can you explain how these relate to the change at hand?

::: browser/themes/windows/browser.css
@@ -1019,5 @@
> -#main-window[tabsintitlebar]:not([inFullscreen]) :-moz-any(#TabsToolbar, #toolbar-menubar) > toolbarpaletteitem > :-moz-any(@primaryToolbarButtons@):-moz-system-metric(windows-classic):not(:-moz-lwtheme),
> -#main-window[tabsintitlebar]:not([inFullscreen]) :-moz-any(#TabsToolbar, #toolbar-menubar) > :-moz-any(@primaryToolbarButtons@):-moz-system-metric(windows-classic):not(:-moz-lwtheme),
> -#home-button.bookmark-item:-moz-lwtheme-brighttext {
> -  position: relative;
> -  z-index: 1;

Why remove the z-indexing + pos: relative? This was required to have things work correctly on top of classic's titlebar gradient. Did you verify that this continued to work correctly on classic?

::: browser/themes/windows/downloads/indicator-aero.css
@@ +7,1 @@
>    margin-bottom: -1px;

Can we go all the way and just delete this file, and move this into a media query in indicator.css ? The comment on its own probably isn't that helpful... We may also want to needinfo mconley to see if we can just get rid of it outright.
Attachment #8424774 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #4)
> - why leave out linux?

I'm not leaving out Linux. This bug should work on Linux and fix bug 947356 / bug 994623 like I mentioned in comment 0.

> - can you try this against our talos suite?

sure

> > +    ToolbarIconColor.inferFromText();
> 
> Can't we listen for an event for the fullscreen case?

I don't know what event that would be.

> Is there a way we can avoid this hitting all the toolbars all the time?

I could easily exclude collapsed toolbars (and call this code when toolbars are made visible).

> Presumably, when tabsintitlebar gets turned off/on, only the tabstoolbar and
> toolbar-menubar (on non-OSX) colors are affected, if at all?

No, we still need inverted icons for lightweight themes and private browsing mode in the future, for instance. Or other crazy modes third party themes or add-on toolbars might introduce. Not duplicating the details of the theme logic in browser.js is a win in my book. We should only worry about this when we see a perf problem.

> Can we build a lightweight caching solution for the active/inactive cases?
> I'm worried about doing sync reflows every time a window gains/loses focus.

To be precise, it's a style flush, and in the common case of [brighttext] or the absence thereof not changing for (de)activate, it's only one flush because nothing gets dirty in the loop.

> Also, have you verified this doesn't duplicate the call you make from
> onLoad? I would expect an activate event on every new window, but perhaps
> that's naive (or it fires before onload?)

I don't the order of these events is guaranteed. I believe these events are triggered by platform-specific widget code; widget code has a history of being fragile in opening windows (e.g. erroneously painting too early).

> > +  observe: function (aSubject, aTopic, aData) {
> > +    switch (aTopic) {
> > +      case "lightweight-theme-styling-update":
> > +        setTimeout(() => { this.inferFromText(); }, 0);
> 
> Why is this in a timeout?

In order to run after LightweightThemeConsumer.jsm's lightweight-theme-styling-update observer.

> > +    function parseRGB(aColorString) {
> 
> If this + the luminance calculations are already somewhere else, can we
> refactor into a toolkit JSM? I expect devtools might also have similar code.
> Should probably followup that to avoid scope creep here though.

Not sure that's worth it considering it boils down to something like 6 lines of code.

> > +      let [r, g, b] = parseRGB(getComputedStyle(toolbar).color);
> > +      let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> > +      if (luminance <= 110)
> > +        toolbar.removeAttribute("brighttext");
> > +      else
> 
> Nit: guard this (!hasAttribute) because setAttributes aren't free.

Should be free except for some extraordinarily small DOM overhead. I don't think that's a kind of micro-optimization we should be doing.

> > +toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention]) > #downloads-indicator-anchor > #downloads-indicator-icon {
> 
> Here, and in several other places related to the download button, you're
> changing the selector, either by adding not clauses, or by changing to/from
> descendant selectors vs. child selectors. Can you explain how these relate
> to the change at hand?

Adding toolbar[brighttext] changes the specificity, so I added :not([attention]) such that I don't overrule the rule changing the icon for [attention].

In other cases, I've changed "#downloads-button #downloads-indicator-icon" to use the child selector, because it should have been like from the start and (since I'm adding toolbar[brighttext]) because two combined descendent selectors are extra bad.

> > -#home-button.bookmark-item:-moz-lwtheme-brighttext {
> > -  position: relative;
> > -  z-index: 1;
> 
> Why remove the z-indexing + pos: relative? This was required to have things
> work correctly on top of classic's titlebar gradient. Did you verify that
> this continued to work correctly on classic?

What does "work correctly" mean here? I still don't understand the purpose of that code (comments would have helped). However, I tested this on classic and it looked good to me.

> ::: browser/themes/windows/downloads/indicator-aero.css
> @@ +7,1 @@
> >    margin-bottom: -1px;
> 
> Can we go all the way and just delete this file, and move this into a media
> query in indicator.css ? The comment on its own probably isn't that
> helpful... We may also want to needinfo mconley to see if we can just get
> rid of it outright.

Since this is unrelated to this bug which I'd like to uplift to aurora, I'll leave this as is, file a bug and needinfo mconley there. I just added the comment such that others stumbling upon this undocumented code don't need to wade through hg history.
> > > +    ToolbarIconColor.inferFromText();
> > 
> > Can't we listen for an event for the fullscreen case?
> 
> I don't know what event that would be.

So there's the fullscreen event, but I'd need to ensure I run after the code in browser-fullScreen.js. Keeping the call in browser-fullScreen.js seems simpler and not too obscure, as I'm adding that API specifically for that purpose.
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > - why leave out linux?
> 
> I'm not leaving out Linux. This bug should work on Linux and fix bug 947356
> / bug 994623 like I mentioned in comment 0.

Err, so why are there no themes/linux/browser.css changes in the patch? E.g.:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1785

looks like it wants changing, too. (admittedly, that's the only thing I found in a quick MXR search - my initial comment was based on the assumption that there would be as much browser.css code as there is on windows, which it seems there isn't, which might be a bug (e.g. the tab overflow arrows and 'show all tabs' button probably want inverting on Linux as well?))


> > > +    ToolbarIconColor.inferFromText();
> > 
> > Can't we listen for an event for the fullscreen case?
> 
> I don't know what event that would be.

I figured the "fullscreen" event? But it seems like you'd need a timeout there, too, so this is probably better, even if I don't like the explicit dependency here.

> > Is there a way we can avoid this hitting all the toolbars all the time?
> 
> I could easily exclude collapsed toolbars (and call this code when toolbars
> are made visible).

This probably makes sense to do.


> > Also, have you verified this doesn't duplicate the call you make from
> > onLoad? I would expect an activate event on every new window, but perhaps
> > that's naive (or it fires before onload?)
> 
> I don't the order of these events is guaranteed. I believe these events are
> triggered by platform-specific widget code; widget code has a history of
> being fragile in opening windows (e.g. erroneously painting too early).

Hum. Sounds like we should file a bug to have tests for that. Having possibly redundant code because otherwise it possibly breaks on some platforms because we don't trust them isn't a great situation to be in. Anyway, followup goop, clearly.

> > > +  observe: function (aSubject, aTopic, aData) {
> > > +    switch (aTopic) {
> > > +      case "lightweight-theme-styling-update":
> > > +        setTimeout(() => { this.inferFromText(); }, 0);
> > 
> > Why is this in a timeout?
> 
> In order to run after LightweightThemeConsumer.jsm's
> lightweight-theme-styling-update observer.

Hrmpf. I imagine we'll need to do things like this more often, and/or add-ons might care as well... can we make the lwt consumer fire a custom event or observer notification in turn? Seems neater than the timeout, but also more work. What do you think?

> > > +    function parseRGB(aColorString) {
> > 
> > If this + the luminance calculations are already somewhere else, can we
> > refactor into a toolkit JSM? I expect devtools might also have similar code.
> > Should probably followup that to avoid scope creep here though.
> 
> Not sure that's worth it considering it boils down to something like 6 lines
> of code.

If it was just 6 lines of code, I would agree, but e.g. the windows 8 frame detection also does some color detection stuff (and, like I said, devtools). It seems like we should just make that available in the platform, and it would be useful for other things (like adding a11y stuff to devtools). Still, followup.

> > > +      let [r, g, b] = parseRGB(getComputedStyle(toolbar).color);
> > > +      let luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
> > > +      if (luminance <= 110)
> > > +        toolbar.removeAttribute("brighttext");
> > > +      else
> > 
> > Nit: guard this (!hasAttribute) because setAttributes aren't free.
> 
> Should be free except for some extraordinarily small DOM overhead. I don't
> think that's a kind of micro-optimization we should be doing.

*shrug*

I don't see why we shouldn't do it. It is clearly non-free, even if the cost is usually small. Anyway, color of the bikeshed. As you like.

> > > +toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention]) > #downloads-indicator-anchor > #downloads-indicator-icon {
> > 
> > Here, and in several other places related to the download button, you're
> > changing the selector, either by adding not clauses, or by changing to/from
> > descendant selectors vs. child selectors. Can you explain how these relate
> > to the change at hand?
> 
> Adding toolbar[brighttext] changes the specificity, so I added
> :not([attention]) such that I don't overrule the rule changing the icon for
> [attention].
> 
> In other cases, I've changed "#downloads-button #downloads-indicator-icon"
> to use the child selector, because it should have been like from the start
> and (since I'm adding toolbar[brighttext]) because two combined descendent
> selectors are extra bad.

Makes sense, thanks for explaining.

> > > -#home-button.bookmark-item:-moz-lwtheme-brighttext {
> > > -  position: relative;
> > > -  z-index: 1;
> > 
> > Why remove the z-indexing + pos: relative? This was required to have things
> > work correctly on top of classic's titlebar gradient. Did you verify that
> > this continued to work correctly on classic?
> 
> What does "work correctly" mean here? I still don't understand the purpose
> of that code (comments would have helped). However, I tested this on classic
> and it looked good to me.

So the foggy fade/gradient/cloud thing that we put on the tabstrip gradient in classic has a z-index. In order for the icons not to be behind it, they needed z-index, and they needed a stacking order. Hence the above two properties. Breaking this would mean that they'd get a blueish hue over the top of the white icon. I'm unsure why you would be able to remove the rules and not break that, but I'll have a look at the next iteration of the patch.

> > ::: browser/themes/windows/downloads/indicator-aero.css
> > @@ +7,1 @@
> > >    margin-bottom: -1px;
> > 
> > Can we go all the way and just delete this file, and move this into a media
> > query in indicator.css ? The comment on its own probably isn't that
> > helpful... We may also want to needinfo mconley to see if we can just get
> > rid of it outright.
> 
> Since this is unrelated to this bug which I'd like to uplift to aurora, I'll
> leave this as is, file a bug and needinfo mconley there. I just added the
> comment such that others stumbling upon this undocumented code don't need to
> wade through hg history.

WFM.
(In reply to Dão Gottwald [:dao] from comment #6)
> > > > +    ToolbarIconColor.inferFromText();
> > > 
> > > Can't we listen for an event for the fullscreen case?
> > 
> > I don't know what event that would be.
> 
> So there's the fullscreen event, but I'd need to ensure I run after the code
> in browser-fullScreen.js. Keeping the call in browser-fullScreen.js seems
> simpler and not too obscure, as I'm adding that API specifically for that
> purpose.

(mid-air, sorry - yes, I figured out essentially the same thing)
(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to :Gijs Kruitbosch from comment #4)
> > > - why leave out linux?
> > 
> > I'm not leaving out Linux. This bug should work on Linux and fix bug 947356
> > / bug 994623 like I mentioned in comment 0.
> 
> Err, so why are there no themes/linux/browser.css changes in the patch? E.g.:

The central pieces for inverting the icons are in browser/themes/shared/toolbarbuttons.inc.css.

> http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1785
> 
> looks like it wants changing, too.

The tab close button SVG is using -moz-MenuBarText on Linux, so this already kind of works.

> (admittedly, that's the only thing I
> found in a quick MXR search - my initial comment was based on the assumption
> that there would be as much browser.css code as there is on windows, which
> it seems there isn't, which might be a bug (e.g. the tab overflow arrows and
> 'show all tabs' button probably want inverting on Linux as well?))

The overflow arrows are using native theming on Linux, which may or may not work well on dark backgrounds and may or may not work well on light backgrounds. I'd rather not touch this here.

> > > Also, have you verified this doesn't duplicate the call you make from
> > > onLoad? I would expect an activate event on every new window, but perhaps
> > > that's naive (or it fires before onload?)
> > 
> > I don't the order of these events is guaranteed. I believe these events are
> > triggered by platform-specific widget code; widget code has a history of
> > being fragile in opening windows (e.g. erroneously painting too early).
> 
> Hum. Sounds like we should file a bug to have tests for that. Having
> possibly redundant code because otherwise it possibly breaks on some
> platforms because we don't trust them isn't a great situation to be in.
> Anyway, followup goop, clearly.

Well, yes... Except that there's no spec for this stuff, so it seems hard to argue that new windows must become active at one particular point regardless of what different window managers on different platforms might do.

> > In order to run after LightweightThemeConsumer.jsm's
> > lightweight-theme-styling-update observer.
> 
> Hrmpf. I imagine we'll need to do things like this more often, and/or
> add-ons might care as well... can we make the lwt consumer fire a custom
> event or observer notification in turn? Seems neater than the timeout, but
> also more work. What do you think?

Might make sense when we actually need to respond to lwt updates more often. I don't really foresee this at the moment.

> > > If this + the luminance calculations are already somewhere else, can we
> > > refactor into a toolkit JSM? I expect devtools might also have similar code.
> > > Should probably followup that to avoid scope creep here though.
> > 
> > Not sure that's worth it considering it boils down to something like 6 lines
> > of code.
> 
> If it was just 6 lines of code, I would agree, but e.g. the windows 8 frame
> detection also does some color detection stuff (and, like I said, devtools).
> It seems like we should just make that available in the platform, and it
> would be useful for other things (like adding a11y stuff to devtools).
> Still, followup.

The frame detection code doesn't parse a CSS color (3 of my 6 lines) and looks at the background in order to determine whether the foreground / text color should be inverted, while I look directly at the text color. Doesn't seem like this can be shared directly. No idea what devtools is doing.

> > > Nit: guard this (!hasAttribute) because setAttributes aren't free.
> > 
> > Should be free except for some extraordinarily small DOM overhead. I don't
> > think that's a kind of micro-optimization we should be doing.
> 
> *shrug*
> 
> I don't see why we shouldn't do it. It is clearly non-free, even if the cost
> is usually small. Anyway, color of the bikeshed. As you like.

Should be a few nanoseconds, i.e. close to free. There's also a cost inherent to micro-optimizations making code more complex.
Attached patch patch v2 (obsolete) — Splinter Review
> > > Is there a way we can avoid this hitting all the toolbars all the time?
> > 
> > I could easily exclude collapsed toolbars (and call this code when toolbars
> > are made visible).
> 
> This probably makes sense to do.

done. I've also explicitly excluded the add-on bar since apparently that's visible all the time, only with zero height...
Attachment #8424774 - Attachment is obsolete: true
Attachment #8425026 - Flags: review?(gijskruitbosch+bugs)
Attached patch patch v3 (obsolete) — Splinter Review
Trying to skip a few redundant ToolbarIconColor.inferFromText calls early in new windows. Will push this to the try server in a bit.
Attachment #8425026 - Attachment is obsolete: true
Attachment #8425026 - Flags: review?(gijskruitbosch+bugs)
Attachment #8425084 - Flags: review?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #10)
> done. I've also explicitly excluded the add-on bar since apparently that's
> visible all the time, only with zero height...

Yeah, this is because if we hid it "properly" the migration code wouldn't work correctly (that is, we wouldn't be able to determine items' width). On top of that, lots of add-ons have a tendency to un-collapse/hide the add-on bar automatically, which would have caused problems if we didn't hide it some other way, too...

(In reply to Dão Gottwald [:dao] from comment #11)
> Created attachment 8425084 [details] [diff] [review]
> patch v3
> 
> Trying to skip a few redundant ToolbarIconColor.inferFromText calls early in
> new windows. Will push this to the try server in a bit.

This is a nice idea. However, looking at the change you made from v2, are we sure that windows can't be opened in the background without an activate/deactivate sequence on any of the OSes we support? It's a bit of an edgecase, but that'd then mean that the icons wouldn't be updated for the deactivated state...

I've not been sleeping enough so I will need to do that soon, but I promise I'll get to reviewing this properly in the morning. Thanks for the awesome work here, it'll clear up a bunch of the ugliness we currently have with the inverted toolbar icons.
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #7 
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1785
> > 
> > looks like it wants changing, too.
> 
> The tab close button SVG is using -moz-MenuBarText on Linux, so this already
> kind of works.

"kind of" ? That sounds suspicious. :-)

> > (admittedly, that's the only thing I
> > found in a quick MXR search - my initial comment was based on the assumption
> > that there would be as much browser.css code as there is on windows, which
> > it seems there isn't, which might be a bug (e.g. the tab overflow arrows and
> > 'show all tabs' button probably want inverting on Linux as well?))
> 
> The overflow arrows are using native theming on Linux, which may or may not
> work well on dark backgrounds and may or may not work well on light
> backgrounds. I'd rather not touch this here.

Fair point, but can you file a bug on that so that we verify that it works and/or fix it? :-)
(In reply to :Gijs Kruitbosch from comment #12)
> This is a nice idea. However, looking at the change you made from v2, are we
> sure that windows can't be opened in the background without an
> activate/deactivate sequence on any of the OSes we support?

I'm not at all sure about that. Since it would be self-repairing (as the user eventually activates the window) and otherwise doesn't seem like an overly harmful glitch, we could start worrying about it if and when we see this happen in the wild...

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > The tab close button SVG is using -moz-MenuBarText on Linux, so this already
> > kind of works.
> 
> "kind of" ? That sounds suspicious. :-)

It should work reliably. It's just a bit weird that close.svg has a dedicated state for menubar-styled toolbars.

> > The overflow arrows are using native theming on Linux, which may or may not
> > work well on dark backgrounds and may or may not work well on light
> > backgrounds. I'd rather not touch this here.
> 
> Fair point, but can you file a bug on that so that we verify that it works
> and/or fix it? :-)

This should already be filed somewhere.
and this version initializes ToolbarIconColor in delayedStartup (which means that the user may see a frame rendered with the wrong icons):

http://compare-talos.mattn.ca/?oldRevs=fc7e2f0b7856&newRev=0f97175ab854&server=graphs.mozilla.org&submit=true
(In reply to Dão Gottwald [:dao] from comment #16)
> and this version initializes ToolbarIconColor in delayedStartup (which means
> that the user may see a frame rendered with the wrong icons):
> 
> http://compare-talos.mattn.ca/
> ?oldRevs=fc7e2f0b7856&newRev=0f97175ab854&server=graphs.mozilla.
> org&submit=true

Thanks for trying all the different approaches. I don't see an immediate gain from this over patch v3 (The ubuntu red looks completely spurious, so I'm ignoring that), and considering the downside you cited, I think v3 is probably the best thing to go with. I'll review the patch right now.
Comment on attachment 8425084 [details] [diff] [review]
patch v3

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

This is almost there. One last question: do you think it's worth writing tests for the JS portion of this?

::: browser/base/content/browser.js
@@ +7253,5 @@
> +      return rgb.map(x => parseInt(x));
> +    }
> +
> +    let toolbars =
> +      document.querySelectorAll("#navigator-toolbox > toolbar:not([collapsed=true]):not(#addon-bar)");

Nit: can you put the selector in a local before the qSA call, and then += the :not([type=menubar]) in an #ifdef XP_MACOSX, in lieu of the analogous block inside the loop?

::: browser/themes/windows/browser.css
@@ +1007,5 @@
>    }
>  }
>  %endif
>  
> +toolbar[brighttext] #home-button.bookmark-item {

Please move this (thoroughly reduced!) rule to toolbarbuttons.inc.css, because the case this is overriding (home button in the bookmarks toolbar + dark lwt) can happen on Linux and OS X as well. You can then get rid of the rule you updated in the line ~625 block in OSX's browser.css entirely, too.

@@ -1019,5 @@
> -#main-window[tabsintitlebar]:not([inFullscreen]) :-moz-any(#TabsToolbar, #toolbar-menubar) > toolbarpaletteitem > :-moz-any(@primaryToolbarButtons@):-moz-system-metric(windows-classic):not(:-moz-lwtheme),
> -#main-window[tabsintitlebar]:not([inFullscreen]) :-moz-any(#TabsToolbar, #toolbar-menubar) > :-moz-any(@primaryToolbarButtons@):-moz-system-metric(windows-classic):not(:-moz-lwtheme),
> -#home-button.bookmark-item:-moz-lwtheme-brighttext {
> -  position: relative;
> -  z-index: 1;

So the reason this continues to work is that there's another rule that sets these properties, which does in fact have a comment about why it's there...

http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#185

That rule has two descendant selectors, and AIUI could be updated to:

#TabsToolbar[brighttext] .toolbarbutton-1

so it has only one.

As for removing these rules entirely, I can't personally visually distinguish the difference on the menubar very well, but the "blue hue" effect is quite pronounced without this CSS if looking at buttons on the classic tabbar.

@@ +1826,5 @@
>    }
>  }
>  %endif
>  
> +#TabsToolbar[brighttext] .tab-close-button:not(:hover):not([selected="true"]) {

I expect this means the rule to invert these buttons in the classic block linked to above can go away, too.
Attachment #8425084 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch patch v4Splinter Review
(In reply to :Gijs Kruitbosch from comment #18)
> This is almost there. One last question: do you think it's worth writing
> tests for the JS portion of this?

Not really, honestly.

> > +    let toolbars =
> > +      document.querySelectorAll("#navigator-toolbox > toolbar:not([collapsed=true]):not(#addon-bar)");
> 
> Nit: can you put the selector in a local before the qSA call, and then +=
> the :not([type=menubar]) in an #ifdef XP_MACOSX, in lieu of the analogous
> block inside the loop?

ok

> > +toolbar[brighttext] #home-button.bookmark-item {
> 
> Please move this (thoroughly reduced!) rule to toolbarbuttons.inc.css,
> because the case this is overriding (home button in the bookmarks toolbar +
> dark lwt) can happen on Linux and OS X as well. You can then get rid of the
> rule you updated in the line ~625 block in OSX's browser.css entirely, too.

Turns out, this isn't needed at all anymore, since we use @primaryToolbarButtons@ (which includes the home button) to set Toolbar.png and Toolbar-inverted.png, rather than .toolbarbutton-1 (which didn't include the home button when moved to the bookmarks toolbar).

> So the reason this continues to work is that there's another rule that sets
> these properties, which does in fact have a comment about why it's there...
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.
> css#185
> 
> That rule has two descendant selectors, and AIUI could be updated to:
> 
> #TabsToolbar[brighttext] .toolbarbutton-1
> 
> so it has only one.

Since this code is about the fog rather than bright text (e.g. when using a lightweight theme), the suggested change doesn't seem to make sense.

> > +#TabsToolbar[brighttext] .tab-close-button:not(:hover):not([selected="true"]) {
> 
> I expect this means the rule to invert these buttons in the classic block
> linked to above can go away, too.

My patch did that already.
Attachment #8425084 - Attachment is obsolete: true
Attachment #8425442 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8425442 [details] [diff] [review]
patch v4

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

Ship it. :-)
Attachment #8425442 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/ff1731d85f58
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment on attachment 8425442 [details] [diff] [review]
patch v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 874674 on Linux, otherwise not a recent regression

User impact if declined: largest impact is for users of high-contrast themes on Windows (attachment 8426129 [details]) and users of dark GTK themes

Testing completed (on m-c, etc.): on m-c

Risk to taking this patch (and alternatives if risky): medium; touches many CSS rules, but fallout (if any) should be minor and easy to fix

String or IDL/UUID changes made by this patch: none
Attachment #8425442 - Flags: approval-mozilla-aurora?
QA Contact: cornel.ionce
Attachment #8425442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1008134
Confirming this fix on latest Nightly, build ID: 20140521030200.
The icons are properly inverted.
(In reply to Cornel Ionce [QA] from comment #26)
> Confirming this fix on latest Nightly, build ID: 20140521030200.
> The icons are properly inverted.

Hi Cornel, based on your comment can the status be changed to 'Verified'
Flags: needinfo?(cornel.ionce)
Hi Marco,

I didn't change the bug status because I've waited for today's Aurora build (20140522004003) so I could also confirm the fix for the 31 branch.

Marking the bug accordingly.

Thank you!
Status: RESOLVED → VERIFIED
Flags: needinfo?(cornel.ionce)
Whiteboard: p=8 s=it-32c-31a-30b.2 [qa+] → p=8 s=it-32c-31a-30b.2 [qa!]
Blocks: 637608
Depends on: 1015572
Depends on: 1061947
Depends on: 1334642
You need to log in before you can comment on or make changes to this bug.