1723938 - Certificate error pages warning title is unreadable in HCM

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Attachment: HCM Black VS HCM#2.png

[Affected versions]:

• Nightly 92.0a1 2021-08-03

[Affected platforms]:

• Win 10 64-bit

[Preconditions]:

• Turn ON High Contrast Mode at system level - with High Contrast Black theme
• Reproducible with all Firefox Themes

[Steps to reproduce]:

1. Open a certificate error page in Firefox (eg. https://expired.badssl.com/)
2. Check that all elements from page are visible in HCM

[Expected result]:

• All elements are readable while in HCM

[Actual result]:

• Warning: Potential Security Risk Ahead label is not readable, it is covered by a white box. The text has different colors in other HCM themes, please see the comparative screenshot.

[Regression range]:

• Found commit message:
  Bug 1720012: Respect system colors checkbox independent of HCM

[Additional notes]:

• Found the issue while testing the [meta] Update certificate error page designs feature, thus I'm blocking also this bug.

Comment 1

[:Gijs (he/him)]

Hey Morgan, do you know what's happening here? If we can fix on the CSS side we can obviously do that, but off-hand I'm not sure why this would have broken as a result of your changes. :-)

Comment 2

[:Gijs (he/him)]

It looks like setting background-color: Window on the h1 that is unreadable here works, but I do not understand why.

The default background-color value for the h1 is transparent, and not changed, and that should work here. Emilio or Morgan, do you know why it doesn't?

(Adding in Micah and Molly for visibility as they're about to start work on a mentored project involving HCM)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So what's going on is that the warning icon has a background-image like this:

  background-image: url("chrome://global/skin/icons/warning.svg");

So when we end up finding a color to backplate, we end up here and choose the default background color, which since the previous bug no longer uses the system colors... But since the system color in use for the text is white, it ends up being unreadable.

So in this case we could either change the break above by a continue (to find the "upper" background-color) or go back to use the system colors in HCM... But probably the first solution is better.

Comment 4

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

So what's going on is that the warning icon has a background-image like this:

  background-image: url("chrome://global/skin/icons/warning.svg");

So when we end up finding a color to backplate, we end up here and choose the default background color, which since the previous bug no longer uses the system colors... But since the system color in use for the text is white, it ends up being unreadable.

So in this case we could either change the break above by a continue (to find the "upper" background-color) or go back to use the system colors in HCM... But probably the first solution is better.

OK, so that might work here, I'll give it a shot, though I'm unsure what else would break...

However, reading this code I have a related question: this loop is informed by the background image, but in this case (ie in about:certerror and friends) there is no visual overlap between the text we're backplating and the background image's size + position. Can/should we detect that case? I suspect it is not uncommon, and it sounds like we could skip based on that, too? (I skimmed nsCSSRendering::DetermineBackgroundColor and it didn't appear to do this either, but perhaps I just missed it?)

Comment 5

[Morgan Reschenberg [:morgan]]

This is a problem in preferences and a few other places too, lemme find the bug for that.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #4)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

So what's going on is that the warning icon has a background-image like this:

  background-image: url("chrome://global/skin/icons/warning.svg");

So when we end up finding a color to backplate, we end up here and choose the default background color, which since the previous bug no longer uses the system colors... But since the system color in use for the text is white, it ends up being unreadable.

So in this case we could either change the break above by a continue (to find the "upper" background-color) or go back to use the system colors in HCM... But probably the first solution is better.

OK, so that might work here, I'll give it a shot, though I'm unsure what else would break...

I don't think it should break much. I think the code was relying on system colors not being honored, which changed along the way, so the foreground color would be the default color.

However, reading this code I have a related question: this loop is informed by the background image, but in this case (ie in about:certerror and friends) there is no visual overlap between the text we're backplating and the background image's size + position. Can/should we detect that case? I suspect it is not uncommon, and it sounds like we could skip based on that, too? (I skimmed nsCSSRendering::DetermineBackgroundColor and it didn't appear to do this either, but perhaps I just missed it?)

Yeah, this seems a bit tricky to do. At best we can do it based on background-size/repeat/position, but seems overcomplicated because the text may be anywhere in the subtree, or there may be e.g. an abspos deep down below which overlaps with the background.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1723938 - Keep looking for background-colors when finding a background-image with transparent background-color. r=morgan

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db4aa060f966
Keep looking for background-colors when finding a background-image with transparent background-color. r=morgan

Comment 9

[Dorel Luca [:dluca]]

Backed out changeset db4aa060f966 (bug 1723938) for Reftest failures in layout/reftests/high-contrast/bg-image-div-002.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=347524781&repo=autoland&lineNumber=8178

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=db4aa060f966a9ff47f1c4773a33f395a378f77a

Backout:
https://hg.mozilla.org/integration/autoland/rev/ba9a969310d7d05bccffc6ab26b3a1f7a8d1f5d3

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77571a2d6d5b
Keep looking for background-colors when finding a background-image with transparent background-color. r=morgan

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/77571a2d6d5b

Comment 12

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on the affected Nightly 92.0a1 (2021-08-03) on Windows 10 x64 and can confirm it as verified-fixed on the latest Nightly 92.0a1 2021-08-09.