Closed Bug 255941 Opened 15 years ago Closed 11 years ago

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

Categories

(Toolkit :: Find Toolbar, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: bugzilla, Unassigned)

References

Details

Attachments

(4 obsolete files)

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:
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.
This might be a dup of bug 56314.
Depends on: 56314
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.
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.
Attachment #157267 - Flags: review?(firefox)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #157267 - Flags: review?(firefox) → review?(bugs)
*** Bug 267429 has been marked as a duplicate of this bug. ***
*** Bug 271653 has been marked as a duplicate of this bug. ***
Same change as the last patch, just updated for new firefox 1.0 findBar.js file
Attachment #157267 - Attachment is obsolete: true
Attachment #157267 - Flags: review?(bugs)
Attachment #169787 - Flags: review?(firefox)
Small formatting change for a one-line patch
Attachment #169787 - Attachment is obsolete: true
Attachment #169787 - Flags: review?(firefox)
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.
Attachment #169789 - Attachment is obsolete: true
Attachment #169793 - Flags: review?(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.
(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) 
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/
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 on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

sorry this took so long.  I could swear I reviewed this already.
Attachment #169793 - Flags: review?(mconnor) → review+
Comment on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

Do it need superreview?
Attachment #169793 - Flags: approval-aviary1.1a1?
(In reply to comment #15)
> Do it need superreview?

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

Attachment #169793 - Flags: approval-aviary1.1a2?
Attachment #169793 - Flags: approval-aviary1.1a1?
Attachment #169793 - Flags: approval-aviary1.1a1-
Comment on attachment 169793 [details] [diff] [review]
make highlighted text black on yellow

a=shaver
Attachment #169793 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
checked-in.
But we have a problem that is background-color contrast problem.
Attachment #169793 - Attachment is obsolete: true
QA Contact: fast.find
Assignee: bross2 → nobody
Product: Firefox → Toolkit
This is fixed by bug 263683.
Status: NEW → RESOLVED
Closed: 11 years ago
Depends on: 263683
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
You need to log in before you can comment on or make changes to this bug.