Closed
Bug 1419152
Opened 7 years ago
Closed 7 years ago
pre-page-render white screen
Categories
(Firefox :: Disability Access, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | verified |
firefox60 | --- | verified |
People
(Reporter: joshudson, Assigned: florian)
References
Details
(Keywords: access, regression)
Attachments
(1 file)
1.02 KB,
patch
|
Gijs
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•7 years ago
|
Has Regression Range: --- → yes
Component: Untriaged → Disability Access
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.
Flags: needinfo?(gwimberly)
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.
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.
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.
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.
Flags: needinfo?(florian)
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.
Blocks: 1379587
Status: UNCONFIRMED → NEW
Has Regression Range: yes → ---
status-firefox57:
--- → fix-optional
status-firefox58:
--- → affected
status-firefox59:
--- → affected
Ever confirmed: true
Flags: needinfo?(gwimberly)
Keywords: access,
regression
Priority: -- → P2
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.
Flags: needinfo?(florian)
Reporter | ||
Comment 9•7 years ago
|
||
I'm now getting the critical security update alarm and have nowhere to go because I cannot update.
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.
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.
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.
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.
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
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?
Flags: needinfo?(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.
Flags: needinfo?(florian)
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.
Updated•7 years ago
|
Attachment #8945531 -
Flags: review?(gijskruitbosch+bugs) → review+
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.
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.
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.
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.
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/57ae6136028a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 25•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(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.
Flags: needinfo?(florian)
Attachment #8945531 -
Flags: approval-mozilla-beta?
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?
Flags: qe-verify+
Flags: needinfo?(florian)
Flags: needinfo?(andrei.vaida)
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.
Flags: needinfo?(florian)
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.
Flags: needinfo?(andrei.vaida)
Comment 30•7 years ago
|
||
Comment on attachment 8945531 [details] [diff] [review] Patch Fix for regression, let's uplift for beta 7.
Attachment #8945531 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 31•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/beb7c47f05a2
Reporter | ||
Comment 32•7 years ago
|
||
Confirmed fixed on debian x64 in Firefox nightly build downloaded 2018-02-05.
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.
Flags: qe-verify+
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Flags: in-qa-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•