Joshua Hudson Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171024165158

Steps to reproduce:

Have; high contrast black
Auto-updated to Firefox 57
Opened a tab

I reported this before and it got mostly fixed before; it has returned in Firefox Quantum much much worse. I have been forced to revert to 56.


Actual results:

Tab (most of screen) turned white and then rendered tab


Expected results:

Tab should have turned black and then rendered content

	Joshua Hudson Reporter
	Comment 1 • 7 years ago

https://en.wikipedia.org/wiki/Photophobia

	:Gijs (he/him)
	Comment 2 • 7 years ago

Grover, can you (find an appropriate QE person to...) look at reproducing this and finding a regression window for this? The reporter seems to be on Windows 7, though it *might* be reproducible on Win8.1 / Win10 with other high contrast themes.

I don't know if this is a performance thing or an about:newtab / activity stream thing, or something else altogether (maybe widget/win32 changes?) but it's not good that this broke and we should try to unbreak it. CC'ing some folks in case they have ideas.

	Florian Quèze [:florian] Assignee
	Comment 3 • 7 years ago

See bug 1379587 comment 26 that seems related. The patch I landed in that bug definitely had something to do with this, but it wasn't really working before my patch either.

	Joshua Hudson Reporter
	Comment 4 • 7 years ago

Known affected: Win7-x86; Win10-x64
Known unaffected: Debian Linux x64

Checking "use system colors" under colors actually makes the bug go away but introduces another bug. (Use system colors is kind of dumb and ends up with a bad link color.)

I would have guessed the pre-erase color is hardcoded white rather than fetching the browser background color, but that obviously isn't right anymore.

The bug isn't performance related. Try opening a new tab to a page that has Auth: Basic and credentials not cached. The tab remains white until credentials are provided to the browser via the popup window.

	Joshua Hudson Reporter
	Comment 5 • 7 years ago

My apologies. I accidentally tested on an old version on Debian Linux. Debian Linux x64 is affected, and my machine is really high performance.

So far I have found no unaffected platforms.

	:Gijs (he/him)
	Comment 6 • 7 years ago

(In reply to Florian Quèze [:florian] from comment #3)
> See bug 1379587 comment 26 that seems related. The patch I landed in that
> bug definitely had something to do with this, but it wasn't really working
> before my patch either.

I don't really understand. This bug looks like it is effectively the same issue as bug 1422507. bug 1379587 suggests that using 'use system colours' is a workaround, which doesn't really make sense to me -- why is that required, why can't we just honour the user-selected foreground and background colours? Presumably we do this in about:newtab, too - it certainly seems like it if I set a custom background colour (e.g. red) and tell Firefox to "always" use my colours over the website's colours (which should be === to using high contrast mode themes on Windows and the default value of the same pref ("Only when using high contrast ...").

Seems like we should just use the colour specified by the user and/or system theme iff that pref would normally override in-page colours anyway. Does that sound sane? Why are we needing to manually do anything here and isn't CSS/gecko taking care of this itself? I tried looking at the patches in the bug you referenced, but I don't see anything that controls what color we use for browsers with the 'blank' attribute.

	:Gijs (he/him)
	Comment 7 • 7 years ago

While we probably won't do a dot-release for this for 57, it seems like a pretty severe regression for users with specific medical conditions that we should fix for 58 if reasonably possible.

	Florian Quèze [:florian] Assignee
	Comment 8 • 7 years ago

(In reply to :Gijs from comment #6)

> Seems like we should just use the colour specified by the user and/or system
> theme iff that pref would normally override in-page colours anyway. Does
> that sound sane?

Yes, when there's a pref set saying to override colors provided by the page, we should also do it for the temporary background when opening new tabs.

> Why are we needing to manually do anything here and isn't
> CSS/gecko taking care of this itself? I tried looking at the patches in the
> bug you referenced, but I don't see anything that controls what color we use
> for browsers with the 'blank' attribute.

browsers with the 'blank' attribute are transparent, so the color that gets shown is the tabbrowser's background.

	Joshua Hudson Reporter
	Comment 9 • 7 years ago

I'm now getting the critical security update alarm and have nowhere to go because I cannot update.

	Joshua Hudson Reporter
	Comment 10 • 7 years ago

User Caspy7 advised using ShadowFox as a workaround.

<Caspy7> https://github.com/overdodactyl/ShadowFox

This does not work, as expected.

	:Gijs (he/him)
	Comment 11 • 7 years ago

(In reply to Joshua Hudson from comment #9)
> I'm now getting the critical security update alarm and have nowhere to go
> because I cannot update.

You can use userChrome.css ( https://www.userchrome.org/how-create-userchrome-css.html ) to override the colour in question. Probably something like:

browser[blank] {
 background-color: #yourdesiredcolourhexvalue !important; /* you can get the hex value from about:config */
 opacity: 1 !important;
}


We should still fix this properly but it's tricky to do this correctly because of various technical reasons.

	Joshua Hudson Reporter
	Comment 12 • 7 years ago

Updating userchrome.css does not address this bug. userchrome.css is not interpreted early enough in the stack. Quick test: change it to red or green then open a new tab to a page that has basic authentication on but you are not currently have saved credentials for.

	Joshua Hudson Reporter
	Comment 13 • 7 years ago

Known affected: Win7-x86; Win10-x64; Debian Linux x64.

	:Gijs (he/him)
	Comment 14 • 7 years ago

(In reply to Joshua Hudson from comment #12)
> Updating userchrome.css does not address this bug. userchrome.css is not
> interpreted early enough in the stack. Quick test: change it to red or green
> then open a new tab to a page that has basic authentication on but you are
> not currently have saved credentials for.

That wfm, I see a flash of red. To make it permanent (probably because about:blank loads in the background) I need to remove the '[blank]' bit of the selector I proposed in comment 11. Make sure you include the `!important` bits, too.

	Florian Quèze [:florian] Assignee
	Comment 15 • 7 years ago

It seems we have 3 similar but different regressions caused by bug 1379587:
- there's a light gray flash with high contrast mode (this bug)
- there's a light gray flash when the background color has been set to black and Override Always has been selected, this is bug 1418814.
- about:blank used as the home page doesn't get the right background color in the first tab of new windows. This is bug 1422507, and probably difficult to fix without first avoiding the spurious about:blank loads we have at startup.

	:Gijs (he/him)
	Comment 17 • 7 years ago

(In reply to Florian Quèze [:florian] from comment #16)
> Created attachment 8945531 [details] [diff] [review]
> Patch

This also changes windows classic (and maybe aero basic, I forget) on Windows 7. Is that OK?

	Florian Quèze [:florian] Assignee
	Comment 18 • 7 years ago

(In reply to :Gijs (lower availability until Jan 27) from comment #17)
> (In reply to Florian Quèze [:florian] from comment #16)
> > Created attachment 8945531 [details] [diff] [review]
> > Patch
> 
> This also changes windows classic (and maybe aero basic, I forget) on
> Windows 7. Is that OK?

It's not perfect, but is equivalent to backing out bug 1379587 there. I think that's OK. I don't have a better solution to offer.

	:Gijs (he/him)
	Comment 19 • 7 years ago

(In reply to Florian Quèze [:florian] from comment #18)
> (In reply to :Gijs (lower availability until Jan 27) from comment #17)
> > (In reply to Florian Quèze [:florian] from comment #16)
> > > Created attachment 8945531 [details] [diff] [review]
> > > Patch
> > 
> > This also changes windows classic (and maybe aero basic, I forget) on
> > Windows 7. Is that OK?
> 
> It's not perfect, but is equivalent to backing out bug 1379587 there. I
> think that's OK. I don't have a better solution to offer.

We should expose nsUXThemeData::IsHighContrast in a media query (or at least via (web)idl and then routed to a window attribute). Anyway, I agree this is better than the status quo.

	Florian Quèze [:florian] Assignee
	Comment 20 • 7 years ago

Thanks for the quick reviews!

(In reply to :Gijs (lower availability until Jan 27) from comment #19)

> We should expose nsUXThemeData::IsHighContrast in a media query (or at least
> via (web)idl and then routed to a window attribute).

This is apparently bug 425598.

	Pulsebot
	Comment 21 • 7 years ago

Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ae6136028a
Avoid white flashes when opening new tabs or windows with high contrast themes, r=Gijs.

	Joshua Hudson Reporter
	Comment 22 • 7 years ago

Ok I have a few more things for you.

The previous directions I was given for installing a custom userchrome were not correct. The file is 'userChrome.css' not 'userchrome.css'. The capital C is required. On setting a userChrome to blue I see the new tab turn blue on Firefox 57, I see the tab turn blue, then white, then whatever color the tab should be from its content.

The loading delay color is still white. Here's another way to reproduce it: http://stackoverflow.com:4444/ (should time out).

Now for the really bad news. nsUXThemeData::IsHighContrast will not turn on on Linux (or if it can, google appears to know no way), so depending on that turned on for the fix is not right.

I don't know what you should do about it but I know what you can do about it: that is, add another environment variable. We already have MOZ_ALLOW_GTK_DARK_THEME=true so adding a MOZ_HIGH_CONTRAST=true is consistent.

	:Gijs (he/him)
	Comment 23 • 7 years ago

(In reply to Joshua Hudson from comment #22)
> The loading delay color is still white. Here's another way to reproduce it:
> http://stackoverflow.com:4444/ (should time out).

Again, with the userChrome suggestion from comment 14 (without the [blank] part of the selector) this works for me.

> Now for the really bad news. nsUXThemeData::IsHighContrast will not turn on
> on Linux (or if it can, google appears to know no way)

Yes, this is bug 239914 / bug 1064164 . It's mostly because Linux doesn't have great (or really, any) infrastructure for detecting high contrast themes from the application level.

Anyway, on Linux people can force-set their colours to override document colours independent from whether high contrast themes are selected, which means the fix from bug 1418814 will solve this issue there, AIUI.

> , so depending on that
> turned on for the fix is not right.

As Florian said in comment #15, we're fixing this in 3 separate places, and bug 1418814 should address this for Linux users who override website colours the same way we automatically do for Windows High Contrast themes.

(In reply to Florian Quèze [:florian] from comment #20)
> Thanks for the quick reviews!
> 
> (In reply to :Gijs (lower availability until Jan 27) from comment #19)
> 
> > We should expose nsUXThemeData::IsHighContrast in a media query (or at least
> > via (web)idl and then routed to a window attribute).
> 
> This is apparently bug 425598.

That one was wontfixed but bug 957988 is still open. IIRC the biggest objection in the past was user fingerprinting etc. and by now we can provide media queries that only work in chrome documents which avoid that objection.

	Andrei Ciure[:aciure]
	Comment 24 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/57ae6136028a

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago

Please request Beta approval on this when you get a chance.

	Florian Quèze [:florian] Assignee
	Comment 26 • 7 years ago

Comment on attachment 8945531 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1379587
[User impact if declined]: light gray flashes when opening a new window or tab for Windows high contrast theme users.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: Looks like it hasn't yet.
[Needs manual test from QE? If yes, steps to reproduce]: Would be nice to have QA do a bit of exploratory testing around opening new tabs and Windows with High Contrast, and/or with various color settings.
[List of other uplifts needed for the feature/fix]: Not strictly required, but it would make sense to uplift bug 1418814 at the same time.
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: simple CSS change. Some area of risk is that this may affect users of the Windows 7 'classic' theme. For them I think it only regresses bug 1379587 without further consequences.
[String changes made/needed]: none.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 27 • 7 years ago

This sounds good, but I agree it could use some exploratory testing. Is this an area we might already cover in upcoming beta testing?

We could either uplift now and do that testing on beta, or test in nightly and then uplift.
Florian, do you think that these issues might be different enough on 59 vs. 60 that we should test one or the other preferentially, or both?

	Florian Quèze [:florian] Assignee
	Comment 28 • 7 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)

> Florian, do you think that these issues might be different enough on 59 vs.
> 60 that we should test one or the other preferentially, or both?

I'm not aware of any difference between 59 and 60 that could affect this. I would focus the exploratory testing on nightly, and on beta verify only that the patches have the expected effect.

	Hani Yacoub, Desktop QA
	Comment 29 • 7 years ago

I did exploratory testing around opening new tabs and Windows with High Contrast, and/or with various color settings on Windows 10 x 64, Windows 7 x32 and Ubuntu 16.04 x64 on the latest nightly 60.0a1 and I didn't find any issues.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 30 • 7 years ago

Comment on attachment 8945531 [details] [diff] [review]
Patch

Fix for regression, let's uplift for beta 7.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/beb7c47f05a2

	Joshua Hudson Reporter
	Comment 32 • 7 years ago

Confirmed fixed on debian x64 in Firefox nightly build downloaded 2018-02-05.

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 33 • 7 years ago

I have reproduced this issue using Firefox  59.0a1 (2017.11.20) on Win 10 x64.
I did exploratory testing around opening new tabs with High Contrast Black settings on Windows 10 x64 and Win 8.1 x64 using Firefox 59.0b8, I didn't found any issues. 
Note: When change the system settings and in meantime Firefox is open, have to restart Firefox to apply the settings.