On Win7 and Win8 the subviews aren't readable on high contrast themes because no system colors are used.

This patch uses for the non-default themes system colors. It also adds for the mainview's buttons a system background-color on hover. The classic theme becomes also this colors as I know no solution how to apply this rules only for HC themes. But the hovered buttons are not really good visible as the mainview background is already without the patch grey and not white like on the most default themes.

Screenshot how it looks with the patch applied.

Fixed also the BMB.

Comment on attachment 8420699 [details] [diff] [review] panelUIHCfix.patch I don't think regressing classic and other non-default themes (custom themes in XP come to mind) in favour of high contrast mode is OK, purely based on the number of people using the different themes. If you think that the end result of doing this is acceptable on classic, then please provide screenshots and request ui-review from shorlander.

(In reply to :Gijs Kruitbosch from comment #4) > Comment on attachment 8420699 [details] [diff] [review] > panelUIHCfix.patch > > If you think that the end > result of doing this is acceptable on classic, then please provide > screenshots and request ui-review from shorlander. (if not, this should depend on getting a selector / media query for high contrast mode. I'm sure there's a bug on this, but I can't find it right now. David?)

Actually, Stephen, can you make a decision as to in what circumstances you want us to hardcode which colors? Right now, I'm not even sure what the desired end state is for the panel. We hardcode quite a lot of colors, but only in some circumstances, it seems. We could fix a couple here, but in the end we'd just be playing whack-a-mole until/unless we make a clear definitive decision about what we want. :-)

This is how the panel would look on Win7 Classic. Note: I hovered multiple buttons to show how they would look. The "Quit" button is still red on hover on all themes..

(In reply to :Gijs Kruitbosch from comment #5) > (In reply to :Gijs Kruitbosch from comment #4) > > Comment on attachment 8420699 [details] [diff] [review] > > panelUIHCfix.patch > > > > If you think that the end > > result of doing this is acceptable on classic, then please provide > > screenshots and request ui-review from shorlander. > > (if not, this should depend on getting a selector / media query for high > contrast mode. I'm sure there's a bug on this, but I can't find it right > now. David?) I think there is talk in standards space but nothing solid AFAIK.

(In reply to :Gijs Kruitbosch from comment #4) > Comment on attachment 8420699 [details] [diff] [review] > panelUIHCfix.patch > > I don't think regressing classic and other non-default themes (custom themes > in XP come to mind) in favour of high contrast mode is OK, purely based on > the number of people using the different themes. If you think that the end > result of doing this is acceptable on classic, then please provide > screenshots and request ui-review from shorlander. Windows lets you customize the colors used for classic and you can't really make any assumptions about custom / third-party themes either. This bug would affect dark classic themes as well as dark third-party themes. That's why we use -moz-windows-default-theme to hardcode the toolbar color on Windows 7, for instance.

(In reply to Richard Marti (:Paenglab) from comment #7) > Created attachment 8421042 [details] > Win7 Classic panelUI > > This is how the panel would look on Win7 Classic. > > Note: I hovered multiple buttons to show how they would look. The "Quit" > button is still red on hover on all themes.. I don't think it's necessary to go that far, I would say that it's good as long as it's readable, no need to make *everything* match. Also, your approach brings out issues, third party themes are badly affected by this.

(In reply to Tim Nguyen [:ntim] from comment #10) > (In reply to Richard Marti (:Paenglab) from comment #7) > > Created attachment 8421042 [details] > > Win7 Classic panelUI > > I don't think it's necessary to go that far, I would say that it's good as > long as it's readable, no need to make *everything* match. > Also, your approach brings out issues, third party themes are badly affected > by this. Do you have an example how this would look?

(In reply to Richard Marti (:Paenglab) from comment #11) > (In reply to Tim Nguyen [:ntim] from comment #10) > > (In reply to Richard Marti (:Paenglab) from comment #7) > > > Created attachment 8421042 [details] > > > Win7 Classic panelUI > > > > I don't think it's necessary to go that far, I would say that it's good as > > long as it's readable, no need to make *everything* match. > > Also, your approach brings out issues, third party themes are badly affected > > by this. > > Do you have an example how this would look? Well, just change the subview background to -moz-field, I'm sure that'll be enough to make the UI readable.

(In reply to Tim Nguyen [:ntim] from comment #12) > > Well, just change the subview background to -moz-field, I'm sure that'll be > enough to make the UI readable. On Classic this would work but on HC-themes there would be no hover feedback. Let's decide Stephen how far this should be changed.

Regressed by: 04b6213da4e1 Mike de Boer ? [Australis] Bug 878546: refresh the styling of sub-views. r=Gijs

Setting tracking 33+ for this regression.

Mike, Gijs, can you help here? Thanks

This needs ui/ux-feedback from Stephen, Philipp, Michael or "someone who isn't an engineer". Adding Philipp to see if that helps with traction. FWIW, I'm not convinced this needs to track 33 - it was shipped in 29-32. We ship lots of hc-incompatible styles, and while that's a problem, I don't think we need to be panicking about this particular bug. (In reply to Richard Marti (:Paenglab) from comment #7) > Created attachment 8421042 [details] > Win7 Classic panelUI > > This is how the panel would look on Win7 Classic. > > Note: I hovered multiple buttons to show how they would look. The "Quit" > button is still red on hover on all themes.. FWIW, this is not good enough at least because the header of the subview now has no contrast - but let's wait with updating patches until we actually have a design/direction here. I suspect that the easiest solution is (as Dão, I think, hinted at in comment #9) to use -moz-windows-default-theme to make all of the *existing* color-based styling we do for the subviews conditional on using one of the "default" themes (that's luna on XP, aero on vista/7, and anything non-high-contrast on windows 8, AIUI). Instead of providing separate styles for non-default-theme and hoping those new styles work in all relevant combinations (which the screenshot shows they don't yet for subview headers, at least...). Alternatively we could override the styling everywhere instead of only a bit (as we do now), but that would likely (a) look bad on certain themes and (b) defeat the purpose of high contrast theme support.

(In reply to :Gijs Kruitbosch from comment #18) > FWIW, I'm not convinced this needs to track 33 - it was shipped in 29-32. OK. Thanks. Not tracking anymore then for 33.

I think setting the background to -moz-field is largely enough. We don't really need hover feedback for now. If we do need so, it's better to do so in a separate bug since none of the UI has hover feedback for HC.

We certainly don't want to regress normal themes (including classic) with the work we are doing on high-contrast themes. I don't think I understand all the complexities here, but it sounds like that's what is happening with the current patch. I'm happy to take a closer look at it if you have a windows build with the patch somewhere (don't have working a windows build environment at the moment).

Try build for test: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/richard.marti@gmail.com-bdf2b27e6a9d/try-win32/

Thanks for the build! Unfortunately, this regresses the classic theme pretty badly. That's probably not news to anyone here, but is there any way to work around this?

(In reply to Philipp Sackl [:phlsa] from comment #23) > Thanks for the build! > > Unfortunately, this regresses the classic theme pretty badly. > That's probably not news to anyone here, but is there any way to work around > this? We have a media query for classic as well (-moz-windows-classic), which we could in theory use together with -moz-windows-default-theme. Dão, does that sound workable?

(In reply to :Gijs Kruitbosch from comment #24) > (In reply to Philipp Sackl [:phlsa] from comment #23) > > Thanks for the build! > > > > Unfortunately, this regresses the classic theme pretty badly. > > That's probably not news to anyone here, but is there any way to work around > > this? > > We have a media query for classic as well (-moz-windows-classic), which we > could in theory use together with -moz-windows-default-theme. Dão, does that > sound workable? No, because on most Windows versions, classic colors is customizable and high-contrast is a classic variant too. (In reply to Tim Nguyen [:ntim] from comment #12) > Well, just change the subview background to -moz-field, I'm sure that'll be > enough to make the UI readable. This sounds about right at least as a first step.

(In reply to Dão Gottwald [:dao] from comment #25) > (In reply to :Gijs Kruitbosch from comment #24) > > (In reply to Philipp Sackl [:phlsa] from comment #23) > > > Thanks for the build! > > > > > > Unfortunately, this regresses the classic theme pretty badly. > > > That's probably not news to anyone here, but is there any way to work around > > > this? > > > > We have a media query for classic as well (-moz-windows-classic), which we > > could in theory use together with -moz-windows-default-theme. Dão, does that > > sound workable? > > No, because on most Windows versions, classic colors is customizable and > high-contrast is a classic variant too. Are you saying that high contrast themes will match the -moz-windows-classic media query? Do we want/need to fix that? I'm aware of the customizability, I just figured that on classic, using browser-specified colors (instead of native colors) for the panels would be an option, just like we do/would for windows-default-theme. Considering UX doesn't think that using native colors yields an acceptable result (which comment #23 seems to imply), that seemed like a sane option.

(In reply to :Gijs Kruitbosch from comment #26) > Are you saying that high contrast themes will match the -moz-windows-classic > media query? Do we want/need to fix that? It's not a bug as far as I can tell, so I don't see why we'd want to fix it. > I'm aware of the customizability, I just figured that on classic, using > browser-specified colors (instead of native colors) for the panels would be > an option, just like we do/would for windows-default-theme. It's an option when done properly; that we get unreadable text with high-contrast themes implies that it's not done properly here. However when done technically correct, it's going to look out of place e.g. when using a dark classic theme (doesn't need to be high-contrast). > Considering UX > doesn't think that using native colors yields an acceptable result (which > comment #23 seems to imply), that seemed like a sane option. That was specifically for attachment 8420699 [details] [diff] [review] / attachment 8421042 [details]. Why would -moz-field be unacceptable? This is what we do for arrow panels on Windows (http://hg.mozilla.org/mozilla-central/annotate/790f41c631cc/toolkit/themes/windows/global/popup.css#l50), why should subviews be different?

I think -moz-field is largely sufficient for this bug, it fixes the summary of this bug. If we want highlights or hover feedback, I'd be better to do so in another bug, since none of our UI has hover feedback (except elements that use native rendering).

(In reply to Tim Nguyen [:ntim] from comment #28) > in another bug, since none of our UI has hover feedback (except elements > that use native rendering). No hover feedback? What is for example this: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#738 ?

Other approach: the hovered buttons have now only a border as feedback. On classic the buttons are still looking as the default theme except the a little bit better visible border. In the subviews the items are still looking like in normal menus. I started a try build for the test: https://tbpl.mozilla.org/?tree=Try&rev=ef974d0fe686

Try from comment 30 failed. Started a new one and the build can be found here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/richard.marti@gmail.com-e0748fb34dd8

(In reply to Richard Marti (:Paenglab) from comment #29) > (In reply to Tim Nguyen [:ntim] from comment #28) > > in another bug, since none of our UI has hover feedback (except elements > > that use native rendering). > > No hover feedback? What is for example this: > http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/ > customizableui/panelUIOverlay.inc.css#738 ? Sorry, I meant no special feedback for high contrast themes.

(In reply to Richard Marti (:Paenglab) from comment #31) > Try from comment 30 failed. Started a new one and the build can be found > here: > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/richard. > marti@gmail.com-e0748fb34dd8 This try build looks good to me.

Comment on attachment 8493981 [details] [diff] [review] panelUIHCfix.patch Review of attachment 8493981 [details] [diff] [review]: ----------------------------------------------------------------- Trying to keep this moving. Dão, I think you're a better reviewer for this patch than I am, do you have time to look at this?

Comment on attachment 8493981 [details] [diff] [review] panelUIHCfix.patch Like Tim and I said, we should just use -moz-field for the background (no -moz-windows-default-theme switch needed). Please try to avoid scope creep and file separate bugs for other things that you think we should improve for high-contrast themes.

This patch change simply the background color to -moz-field on Windows.

Comment on attachment 8499110 [details] [diff] [review] Use -moz-field -moz-field is our standard arrow panel background on Windows, so it's not clear to me why you'd need to specify it here at all. Where does the current hardcoded white background come from?

From here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#102

What happens if you just remove the background color there?

Removing the color with color: transparent? It would look like the screenshot.

I mean background-color: transparent.

Comment on attachment 8499110 [details] [diff] [review] Use -moz-field Ok, looks like there's no way around this then. However, at this point panelUIOverlay.inc.css sets the background only for OS X, because Windows and Linux would override it. Can you move that background to browser/themes/osx/customizableui/panelUIOverlay.css?

Moved the background color definition to platform files.

Comment on attachment 8499380 [details] [diff] [review] Bug1008603.patch >--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css >+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css > .panel-subviews { > padding: 4px; >- background-color: hsla(0,0%,100%,.97); > background-clip: padding-box; > border-left: 1px solid hsla(210,4%,10%,.3); > box-shadow: 0 3px 5px hsla(210,4%,10%,.1), > 0 0 7px hsla(210,4%,10%,.1); > color: hsl(0,0%,15%); > -moz-margin-start: @exitSubviewGutterWidth@; > } Please also remove 'color'. r+ with that

Removed the color definition in panelUIOverlay.inc.css.

Comment on attachment 8499406 [details] [diff] [review] patch for check-in [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: not readable panels on high contrast Windows themes until the next ESR release. Fix Landed on Version: 35 Risk to taking this patch (and alternatives if risky): low, only CSS change String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

It seems that the patch ignores Linux. I might be wrong though. Should the color definition be in Linux too ?

(In reply to Richard Marti (:Paenglab) from comment #48) > Comment on attachment 8499406 [details] [diff] [review] > patch for check-in > > [Approval Request Comment] > If this is not a sec:{high,crit} bug, please state case for ESR > consideration: > User impact if declined: not readable panels on high contrast Windows themes > until the next ESR release. > Fix Landed on Version: 35 > Risk to taking this patch (and alternatives if risky): low, only CSS change > String or UUID changes made by this patch: none > > See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more > info. I don't think this should be uplifted before it's even been in Nightly, esp. as what ended up happening was that after I said we should get to UX to look at this, and then they did, we landed a different patch without looping them back in. As I already noted in comment #18, we've shipped this for ages, we don't need to rush this in now. There's plenty of time for 34, and 33 has kind of sailed already (we're past the last beta).

Comment on attachment 8499406 [details] [diff] [review] patch for check-in This is not a driver for a new 32 release and too late for beta (we are going to publish beta 9 today). But taking it for aurora. I don't think we are going to take it for ESR but leaving the call to the ESR driver.

Comment on attachment 8499406 [details] [diff] [review] patch for check-in Gijs comment arrived 10 seconds before mine. Updating the approval flag for aurora

(In reply to Tim Nguyen [:ntim] from comment #49) > It seems that the patch ignores Linux. I might be wrong though. Should the > color definition be in Linux too ? Linux has already a definition: http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/linux/customizableui/panelUIOverlay.css#7

Comment on attachment 8499406 [details] [diff] [review] patch for check-in Review of attachment 8499406 [details] [diff] [review]: ----------------------------------------------------------------- I agree with feedback in Comment 50 and 51 and do not want to rush this into ESR.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 The panelUI subview is now readable using High Contrast themes in Firefox Nightly, build ID: 20141005030205. Was there a bug already filled for the hover feedback (which is not yet implemented) or should I file a new one?

It is not yet filed. Thanks if you can file.

Comment on attachment 8499406 [details] [diff] [review] patch for check-in Now that this has baked on m-c for a few days, Aurora+.

https://hg.mozilla.org/releases/mozilla-aurora/rev/eff0d9ee1b1a Resetting the esr31 tracking request since it's unclear to me if this is wontfix for esr31 for good or just until the next release cycle.

(In reply to Richard Marti (:Paenglab) from comment #56) > It is not yet filed. Thanks if you can file. I've logged bug 1079098 for the missing hover feedback.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Verified fixed using latest Aurora, build ID: 20141009004002.