6.19 KB, patch
|Details | Diff | Splinter Review|
5.62 KB, patch
|Details | Diff | Splinter Review|
4.92 KB, text/plain
417.27 KB, text/html
1.15 KB, patch
|Details | Diff | Splinter Review|
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.
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?
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.
(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.
Created attachment 334579 [details] [diff] [review] 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.
Created attachment 334583 [details] [diff] [review] 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
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?)
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
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.
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?
For https://bugzilla.mozilla.org/attachment.cgi?id=334555 I get a green "Highlight All" background color and black text color, btw.
Created attachment 338130 [details] [diff] [review] 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?
Created attachment 338435 [details] 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.
Created attachment 338506 [details] [diff] [review] 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
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.
Is bug 455874 related to this one?
No, that's bug 455217.
(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.
Created attachment 345739 [details] [diff] [review] 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.
Created attachment 345741 [details] Backgrounds with sufficient contrast For reference, these are the backgrounds whose luminosity difference from white >=125000 (the level at which it flips in Windows)
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 on attachment 345739 [details] [diff] [review] Follow-up v2 - new highlight colour let's see what the masses think
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>
Created attachment 347345 [details] [diff] [review] 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 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 on attachment 347345 [details] [diff] [review] Follow-up v2a - new highlight colour [pushed comment 29] Please get this in ASAP.
Comment on attachment 334583 [details] [diff] [review] Patch v2 [pushed comment 6] This looks like what landed in comment 6.
>(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 on attachment 347345 [details] [diff] [review] Follow-up v2a - new highlight colour [pushed comment 29] http://hg.mozilla.org/mozilla-central/rev/27c3c042ffb7
-> v. Thank you, Graeme!