255941 - Highlight colour / color does not contrast with document text or background

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Ben Basson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040817 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040817 Firefox/0.9.1+

The Find Toolbar's highlight feature uses yellow highlighting, something which
is hardcoded.

This is a problem on two occasions:

1) When the background is yellow, or a colour that is near to yellow.
   http://www.cusser.net/misc/firefox/highlight/testcase.php

2) When the text is yellow, or a colour near to yellow.
   http://www.cusser.net/misc/firefox/highlight/testcase2.php

In the first case, the highlighted text is hardly distinguishable or completely
indistinguishable from regular text... In the second case, the text is hardly
visible or totally invisible.

In my Context Highlight extension
(http://extensions.cusser.net/contexthighlight/contexthighlight.xpi) I checked
through DOM traversal to find the true background colour behind the text, and
chose an appropriate background colour to contrast with it, using the following
code:

if (colorString.substring(0,3) == 'rgb') {
            
  colorString = colorString.substring(4,(colorString.length-1));
  colorString = colorString.replace(" ","");
  var sArray = colorString.split(",");          
              
  if ((sArray[0] == 255) && (sArray[1] >= 200) && (sArray[1] <= 255))
contrastRequired++;
  if ((sArray[1] == 255) && (sArray[0] >= 000) && (sArray[0] <= 255))
contrastRequired++;
         
}

While there are prettier and more comprehensive ways of doing this... I believe
that we should discuss and implement something similar, since this is a pretty
major oversight in the highlight algorithm.

Marking major due to the fact that this renders Firefox's highlighter nearly
useless on some pages.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Michael Clackler]

Possibly we need to do something like the recent fix for bug 249680 to fix light
text over a yellow background on the URL bar.  This would not fix the state
where the highlight color is close to the page background color.

I was also thinking about putting the highlight color into a user-pref since it
would be nice if the end-user could change the color to help those who have
trouble seeing yellow.

Comment 2

[Jesse Ruderman]

This might be a dup of bug 56314.

Comment 3

[Ben Basson]

I wouldn't have thought so, since bug 56314 is about the colour of mouse
selections. This bug is about the highlight colour (yellow) used by the Find
Toolbar.

Comment 4

[Michael Clackler]

Attachment: patch to make all text black on yellow

This patch doesn't fix the highlight color contrasting problem, but it will
does fix the problem of colored text being seen on the yellow highlight.  I
have to think some more about (and figure out how to..) fix the problem of the
highlight color contrasting problem.

The color or all text highlighted is changed to black (like the recent change
for secure URL's) to ensure visibility of the highlighted string.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

*** Bug 267429 has been marked as a duplicate of this bug. ***

Comment 6

[Phil Ringnalda (:philor)]

*** Bug 271653 has been marked as a duplicate of this bug. ***

Comment 7

[Michael Clackler]

Attachment: updated patch to make highlighted text black on yellow

Same change as the last patch, just updated for new firefox 1.0 findBar.js file

Comment 8

[Michael Clackler]

Attachment: make highlighted text black on yellow

Small formatting change for a one-line patch

Comment 9

[Michael Clackler]

Attachment: make highlighted text black on yellow

Updated patch with suggestions from Gavin to put the hard-coded text color in
the same location as the hard-coded background color.  This makes future
changes to colors/possible additions of preferences easier.

Comment 10

[Mike Connor [:mconnor]]

this is okay, IF we don't care about yellow backgrounds.  However, if this is
going to be a "just good enough to make the real fix unnecessary" situation,
maybe we shouldn't go this way on things.

Also, isn't the find highlighting hardcoded as well?

Flipside to any dynamic highlighting solution is that we may run into problems
if the highlight/find colours aren't consistent enough.  This requires more
thought than 5 AM allows for, so I'll hold off on r+

the patch looks fine though.

Comment 11

[Michael Clackler]

(In reply to comment #10)
> this is okay, IF we don't care about yellow backgrounds.

When I started looking into this, I could find no way to get the background
(RGB) color of selected text.

I was thinking about adding 4 preferences for:
  Highlight Backgroung Color 1
  Highlight Text Color 1
  Highlight Backgroung Color 2
  Highlight Text Color 2

Then setting background color 1 to yellow, and background color 2 to a
light-blue (Both texts black).  Between yellow and blue we can visibly highlight
any background color.  The reason for the preferences was to allow people with
color-sensitivity problems to change them to a color more suited to them.

At the time there was some backlash about there being to many preferences, and
the overhead cost of loading them getting high.  Since I could find no way to
discover the background color of selected text, there really was no reason to
add them.

The current patch does not solve the background color situation, but personally
in my browsing I find the text color issue a much more promenent problem (light
text on dark backgrounds).  With Gavin's suggestion of putting the hard-coded
colors in the same area, any future dynamic color changes would still probably
be able to use the code in this patch (as long as we stay with .js) 

Comment 12

[Ben Basson]

It's easy to get the background colour of text nodes, I employ a few short
functions in my Context Highlight extension (version 0.2) to establish the RGB
values. 

It's probably more comprehensive than anything I'd expect to be put into Firefox
by default, but if you're working on a patch for this bug, it's probably worth
checking out...

You can find it at http://www.cusser.net/extensions/contexthighlight/

Comment 13

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

I have a following idea.

1. set text color. (e.g., color=black, bgcolor=yellow)
2. add new css property that name is "-moz-auto-color-reverse".
3. change highlight element to "<span id="__firefox-findbar-search-id"
style="color: black; background-color: yellow; -moz-auto-color-reverse: auto;">".
4. in nsTextFrame.cpp, if -moz-auto-color-reverse is auto, check that the
background color and text color has suficient contrast by same algorithm as bug
56314.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

sorry this took so long.  I could swear I reviewed this already.

Comment 15

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

Comment on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

Do it need superreview?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> Do it need superreview?

No superreview is needed for toolkit, see
http://www.mozilla.org/projects/toolkit/review.html .

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

a=shaver

Comment 18

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

checked-in.
But we have a problem that is background-color contrast problem.

Comment 19

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

This is fixed by bug 263683.