451204 - Highlighted text is white on white text on a yellow background, difficult to read

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Martijn Wargers (dead)]

The patch for bug 263683 made (for me at least) highlighted text white text on a yellow background, while it used to be black text on a yellow background.
White text on a yellow background is much more difficult to read than black text on a yellow background.

Also, I noticed that you can change the highlight selection color by using the ui.textHighlightBackground pref, which is really cool.
But I can't find a ui.textHighlightColor pref or something like that, that would enable me to choose the text highlight color.
Currently, I can't choose a white as a highlight color, for instance.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I cannot see this on OS X with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080819020844 Minefield/3.1a2pre ID:20080819020844

The color of the highlighted text is still black on a yellow background. It doesn't turn into white. So a Windows only issue? Aleksej, can you reproduce this on Linux?

Comment 2

[[:Aleksej]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2pre) Gecko/20080819021828 Minefield/3.1a2pre

The found text is highlighted as white on green.

Comment 3

[[:Aleksej]]

(follow-up to comment #2)
And black on yellow-white-yellow-white is what the Find bar's field blinks when starting to search first.

Comment 4

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

Yeah, the foreground colour it takes is the system foreground colour for selection, which on XP/Vista is going to be white, and is going to suck.

Trivial patch to explicitly make the foreground colour black, for improved contrast/readability, and allow customizing it through a ui pref.

Comment 5

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2 [pushed comment 6]

Now with the (admittedly disabled) reftest updated. Even if the reftest were working, don't think this would warrant a separate test

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/eb17a09b9b9a

Comment 7

[Martijn Wargers (dead)]

Now, in current trunk build, the background color is black and the text color is yellow. Is that what you wanted, Graeme? (and not the other way around?)

Comment 8

[Graeme McCutcheon [:graememcc]]

Martjin, what is the current background colour of the word you are highlighting, before you use the find toolbar? The selection painting code will flip the foreground/background colours for maximum contrast, depending on the page's background colour. (This is why the colour I picked isn't the standard yellow colour, as this "flip" was occuring on pages with a white background!).

Ultimately, no matter what colour is chosen, some pages will cause this - without it, we are back at bug 255941

Comment 9

[Martijn Wargers (dead)]

I was just testing on this bugzilla page, so background color: white with black text color.

In Firefox 3, the highlighted color was always background color: yellow with black text color.
With builds after this patch went in, indeed the highlight colors seem to depend on the background color, which is cool! Thanks for that.

But now for  background color: white with black text color pages (like this bugzilla page in general), the highlight colors are now background-color: black with yellow text color.
This is a change of color of what it was in Firefox 3. Perfectly fine by me, if it's fine by you.

Comment 10

[Graeme McCutcheon [:graememcc]]

No, that doesn't sound very nice at all. Works OK on Ubuntu, though.

roc: any idea what's happening here? The sufficient luminosity value is platform independent, so why would this calculation be giving different results on different platforms?

Comment 11

[Martijn Wargers (dead)]

For https://bugzilla.mozilla.org/attachment.cgi?id=334555 I get a green "Highlight All" background color and black text color, btw.

Comment 12

[Graeme McCutcheon [:graememcc]]

Attachment: The printf school of debugging

Hmm. I have the beginnings of an idea.

Martijn: I don't have easy access to a Windows box. The attached patch, aside from having no regard for any type of coding standards, sends some info about how it's deciding to flip the colours. Would you be able to apply this patch, run the build with -console, perform a "highlight all" on a page where the matches are on a white background, and post the output here?

Comment 13

[Martijn Wargers (dead)]

Attachment: output

Sorry for the delay, this is the output I get when looking at https://bugzilla.mozilla.org/attachment.cgi?id=338130 when highlighting the word 'void'. I get a black background color and yellow text color for the highlight.

Please, ignore comment 11, I had the color prefs flipped in there in that profile. Sorry about that.

Comment 14

[Graeme McCutcheon [:graememcc]]

Attachment: Follow-up patch

Martijn, thanks for your help.

So, the problem lies in the calculation of what constitutes sufficient contrast, before the colours are flipped. Sufficient contrast is calculated as the minimum of three values: 

- the constant NS_SUFFICIENT_LUMINOSITY_DIFFERENCE
- the luminosity difference between the selection foreground and background colours
- the luminosity difference between the selection background colour and the window background colour.

On Ubuntu, the second of those gives a very low value, and the difference between the highlight BG colour and white was larger, so no flipping occurs. On Windows, this is not the case, and as can be seen from Martijn's output NS_SUFFICIENT_LUMINOSITY_DIFFERENCE was being picked. The highlight luminosity was lower, causing the flip on white backgrounds.

As the highlight has nothing to do with normal selection, it makes no sense to depend on normal selection colours when calculating sufficient contrast. This patch makes the obvious changes. It also gives me the opportunity to change the highlight colour back to the yellow it used to be before I came along

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would simplify the code a bit, and not slow things down in practice, if we didn't cache the mSufficientContrast* values in InitCommonColors, but just calculated them in EnsureSufficientContrast as needed.

> As the highlight has nothing to do with normal selection, it makes no sense to
> depend on normal selection colours when calculating sufficient contrast.

I don't quite follow this. I thought the idea of this code was that the platform's luminosity difference between selected text and selection background should be "enough" for the user, and likewise the platform's luminosity difference between selection background and window background should also be "enough" for the user. So in that sense, it is relevant to the color choices for highlighting; we don't need to make highlighting stand out any more than the platform's selection already stands out.

So I'm not really sure why we need to do this. I'm a bit afraid we might be poking the algorithm to get aesthetically pleasing colors in one situation and break other situations.

Comment 16

[Dão Gottwald [:dao]]

Is bug 455874 related to this one?

Comment 17

[Martijn Wargers (dead)]

No, that's bug 455217.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> > As the highlight has nothing to do with normal selection, it makes no sense to
> > depend on normal selection colours when calculating sufficient contrast.
> 
> I don't quite follow this. I thought the idea of this code was that the
> platform's luminosity difference between selected text and selection background
> should be "enough" for the user, and likewise the platform's luminosity
> difference between selection background and window background should also be
> "enough" for the user.

Exactly, I have designed so. The contrast between selection background color and window background color, and also selection background color and selection foreground color should have enough contrast for the user.

> So in that sense, it is relevant to the color choices
> for highlighting; we don't need to make highlighting stand out any more than
> the platform's selection already stands out.
> 
> So I'm not really sure why we need to do this. I'm a bit afraid we might be
> poking the algorithm to get aesthetically pleasing colors in one situation and
> break other situations.

Yes. I think that we should use darker color for the background color. However, it means to decrease the contrast between highlight background color and highlight foreground color. The luminosity difference between yellow like colors and white are not enough for accessibility. We should use non-yellow like color, or we should use dark yellow color and white foreground color.

Comment 19

[Graeme McCutcheon [:graememcc]]

Attachment: Follow-up v2 - new highlight colour

(In reply to comment #18)
> The luminosity difference between yellow like
> colors and white are not enough for accessibility. We should use non-yellow
> like color, or we should use dark yellow color and white foreground color.

I found even darker yellows still quite hard to read with white text. Writing a little program to do the math, and output a page showcasing the possible colours, the available highlight background colours were basically blacks, dark oranges/browns, some reds, light greens, and blues and purples.

I worked to the constraints:
1) It shouldn't flip on white backgrounds on windows, 
2) It shouldn't be red for the warning connotations of that colour, 
3) Blue would be no good if it was too close to the standard windows selection colour, as highlights should be discernably different
4) Likewise for greens that are close to the SELECTION_ATTENTION colour
5) It shouldn't be black

I've chosen a pinkish-purple colour. Given this will cause a minor UI change in Firefox, I'll cc in mconnor for his view.

Comment 20

[Graeme McCutcheon [:graememcc]]

Attachment: Backgrounds with sufficient contrast

For reference, these are the backgrounds whose luminosity difference from white >=125000 (the level at which it flips in Windows)

Comment 21

[Graeme McCutcheon [:graememcc]]

Mike, cc'ing you on this.

The 1 comment story so far: since landing bug 263683, the yellow background previously used for highlighting no longer works on common cases (eg pages with white backgrounds): the Gecko contrast-checking code flips the text and background colours so that the highlighting becomes yellow text on a black background.

Per comments above, the solutions are either choosing a new highlight colour, or backing out 263683 and going back to the "bad" way of performing highlighting. 

Obviously choosing a new colour is a significant user-facing change.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 345739 [details] [diff] [review]
Follow-up v2 - new highlight colour

let's see what the masses think

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

mmm, Isn't better the white text for the new highlight text?

data:text/html,<span style="background-color:#ef0fff;">highlight text</span><br/><span style="background-color:#ef0fff;color: white;">highlight text</span>

Comment 24

[Graeme McCutcheon [:graememcc]]

Attachment: Follow-up v2a - new highlight colour [pushed comment 29]

> mmm, Isn't better the white text for the new highlight text?

OK. Assuming I don't need another round of r/sr from roc for a further trivial change, so carrying forward r+sr=roc.

Rerequesting review from mconnor to check he's OK with this.

Requesting approval for b2: per mconnor in comment#22, I assume we want to get feedback on this as soon as possible. Additionally, this is a very low risk change, purely cosmetic: the code that manages drawing these selections has been in the tree since end of July.

Comment 25

[Graeme McCutcheon [:graememcc]]

Comment on attachment 347345 [details] [diff] [review]
Follow-up v2a - new highlight colour [pushed comment 29]

Er, stupidity sorry. Will get the r+ first before I ask for approval...

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 347345 [details] [diff] [review]
Follow-up v2a - new highlight colour [pushed comment 29]

Please get this in ASAP.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 334583 [details] [diff] [review]
Patch v2 [pushed comment 6]

This looks like what landed in comment 6.

Comment 28

[Graeme McCutcheon [:graememcc]]

>(From update of attachment 334583 [details] [diff] [review])
>This looks like what landed in comment 6.

The patch your linking to is indeed what landed in comment 6. The patch added in comment 24 is the one that requires landing.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 347345 [details] [diff] [review]
Follow-up v2a - new highlight colour [pushed comment 29]

http://hg.mozilla.org/mozilla-central/rev/27c3c042ffb7

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> v.

Thank you, Graeme!