Last Comment Bug 56314 - reverse selection colors when page background is similar to default selection background
: reverse selection colors when page background is similar to default selection...
Status: RESOLVED FIXED
Don't add comment for new problem. Se...
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: P1 normal with 9 votes (vote)
: mozilla1.8beta2
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
: Terri Preston
Mentors:
: 15286 36001 91708 140580 156939 162478 164703 175685 184277 208693 241657 254844 (view as bug list)
Depends on: 292191 289652 322798
Blocks: 234102 290919 113161 140580 164421 176605 255941 289191 289287
  Show dependency treegraph
 
Reported: 2000-10-12 14:41 PDT by Scott Gifford (old account)
Modified: 2014-04-26 03:11 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (256 bytes, text/html)
2002-05-13 19:43 PDT, Jesse Ruderman
no flags Details
Patch (5.35 KB, patch)
2004-11-05 06:20 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv2 (5.71 KB, patch)
2004-11-05 22:59 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
testcase (178.11 KB, text/html)
2004-11-05 23:18 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
Patch rv2.1 (5.68 KB, patch)
2004-11-06 01:27 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
another testcase (528 bytes, text/html)
2005-01-30 06:05 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
Patch rv3.0(work in progress) (6.09 KB, patch)
2005-02-01 06:08 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv4.0 (6.52 KB, patch)
2005-02-01 07:54 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Patch rv5.0 (8.80 KB, patch)
2005-02-02 11:14 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv6.0 (14.46 KB, patch)
2005-02-03 03:38 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv6.1 (14.44 KB, patch)
2005-02-03 07:31 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Patch rv6.2 (17.49 KB, patch)
2005-02-04 03:43 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv7.0 (17.17 KB, patch)
2005-02-15 02:42 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
sfraser_bugs: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch for check-in (17.07 KB, patch)
2005-04-03 13:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch for disable previous patch (1.80 KB, patch)
2005-04-06 05:49 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
testcase (257.11 KB, text/html)
2005-04-06 07:35 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
testcase2 (276.60 KB, text/html)
2005-04-06 07:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
Patch rv8.0 (6.71 KB, patch)
2005-04-06 10:49 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv8.1 (6.71 KB, patch)
2005-04-06 11:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv8.2 (6.27 KB, patch)
2005-04-07 11:03 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch for check-in (6.27 KB, patch)
2005-04-07 11:14 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch for check-in (6.27 KB, patch)
2005-04-07 11:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv9.0 (6.57 KB, patch)
2005-04-08 10:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch rv10.0 (7.51 KB, patch)
2005-04-08 11:06 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bzbarsky: review+
bzbarsky: superreview+
dbaron: approval1.8b2+
Details | Diff | Splinter Review
On colorful device, monochrome device and if the user color-blindness(2 patterns) (1.54 KB, image/png)
2005-04-08 23:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
screen shot of comment 108 (285.77 KB, image/jpeg)
2005-04-08 23:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details
Patch rv11.0(test patch, I don't like this) (12.91 KB, patch)
2005-04-10 11:50 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
screenshot: comparison of the selection before&after the patch (31.51 KB, image/png)
2005-04-20 19:12 PDT, rbs
no flags Details
screenshot (87.28 KB, image/jpeg)
2005-04-21 01:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details

Description Scott Gifford (old account) 2000-10-12 14:41:16 PDT
Mozilla should be more clever about dynamically detecting textSelectBackground.
 If text is selected in an area where textSelectBackground is the same, or very
close to, the background color, Mozilla should dynamically change
textSelectBackground for that one selection, handling the case where different
text in the same selection has different background colors correctly.

I ran into a problem with this when my textSelectBackground was somehow detected
as white (see Bug #56076), and selections became almost unusable.
Comment 1 Akkana Peck 2000-10-12 15:20:36 PDT
Confirming (it's definitely a problem) and joining cc.  I think we already have
a bug somewhere on this, but I'm not sure of the bug # so I'll let someone else
dup it if there really is one.
Comment 2 Blake Ross 2000-10-12 15:23:16 PDT
hmm..I don't know of one. Looking...
Comment 3 Simon Fraser 2000-10-13 13:42:41 PDT
Not going to get to this by RTM, future.
Comment 4 Blake Ross 2002-02-16 13:15:08 PST
changing selection qa to tpreston.
Comment 5 Jesse Ruderman 2002-05-13 19:16:48 PDT
*** Bug 91708 has been marked as a duplicate of this bug. ***
Comment 6 Jesse Ruderman 2002-05-13 19:43:12 PDT
Other browsers, when selecting white-on-blue text:

Netscape 4   blue-on-white selection (only for <body> and not for <p>)
IEWin 6      blue-on-white selection
Opera 6      selection inverts colors of contents; there is no selection color

IEWin also switches to blue-on-white selection when selecting yellow-on-white
text.  Maybe it does this to compensate for pages that set the background color
dark using an image rather bgcolor or background-color.  (It's easy to tell what
the text color is, but poorly coded pages may lie about the background color.)

Opera always inverts the selection contents.  Black-on-white text becomes
white-on-black (doesn't match Windows platform), and yellow-on-white becomes
blue-on-black (barely more readable than yellow-on-white).
Comment 7 Jesse Ruderman 2002-05-13 19:43:43 PDT
Created attachment 83458 [details]
testcase
Comment 8 Mats Palmgren (:mats) 2002-07-11 12:47:34 PDT
*** Bug 156939 has been marked as a duplicate of this bug. ***
Comment 9 Jesse Ruderman 2002-08-13 11:14:20 PDT
*** Bug 162478 has been marked as a duplicate of this bug. ***
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2002-08-27 21:29:20 PDT
*** Bug 164703 has been marked as a duplicate of this bug. ***
Comment 11 Jesse Ruderman 2002-10-20 17:07:58 PDT
*** Bug 175685 has been marked as a duplicate of this bug. ***
Comment 12 Tom Mraz 2002-11-01 14:41:49 PST
4xp as per comment 6
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2003-01-26 03:10:48 PST
In CSS3 selectors module.
http://www.w3.org/TR/css3-selectors/#UIfragments

Bug 176170
:-moz-selection bugs tracking bug (::selection)

If a page has this pseudo-elements having (background-)color properties,
Mozilla should use css values.

So, This problem should be fixed when the page don't have there properties.
Comment 14 Simon Montagu :smontagu 2003-02-27 15:04:42 PST
*** Bug 184277 has been marked as a duplicate of this bug. ***
Comment 15 Jesse Ruderman 2003-07-14 22:31:32 PDT
*** Bug 36001 has been marked as a duplicate of this bug. ***
Comment 16 Jesse Ruderman 2003-07-14 22:34:07 PDT
See also bug 111526, "link focus outline (invert) invisible on gray backgrounds".
Comment 17 Bogdan Stroe 2004-04-25 15:54:35 PDT
*** Bug 241657 has been marked as a duplicate of this bug. ***
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2004-08-08 18:27:32 PDT
*** Bug 254844 has been marked as a duplicate of this bug. ***
Comment 19 Jesse Ruderman 2004-08-18 10:47:29 PDT
*** Bug 15286 has been marked as a duplicate of this bug. ***
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 06:20:45 PST
Created attachment 164736 [details] [diff] [review]
Patch

I created the patch.
But it conflict with bug 234102.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 06:46:25 PST
My algorithm is not equal IE.
Because I don't know the algorithm of IE.
But I think that my algorithm is similar to IE.

This patch is fixed this bug, bug 140580, and bug 176605.

The algorithm is simple.

1. We determine pseudo-background-color by the foreground-color.
Because the "real" background-color is not always set by author.
e.g., author set the following rules.
foo{
  color: white;
  background-image: black.png;
}

2. Calculate the two distance between the two point in the color space.
The one is between foreground-color of selection and pseudo-background-color(a).
Other is between background-color of selection and pseudo-foreground-color(b).

3. If the distance of (a) is shorter than (b), we use normal selection color.
In other case, we exchange the background-color of selection and
foreground-color of selection.

I arranged the way of 2 and 3.
It is for IE compatible(but not equal).

2:
COLOR_DISTANCE(colorA, colorB) (
        (
          NS_GET_R(colorB) - NS_GET_R(colorA)
          + NS_GET_G(colorB) - NS_GET_G(colorA)
        ) * 2
        + NS_GET_B(colorB) - NS_GET_B(colorA)
)

3:
if (abs(COLOR_DISTANCE(selectionFGColor, pseudoBGColor)) <=
    abs(COLOR_DISTANCE(selectionBGColor, pseudoBGColor)) * 2)
                                                         ~~~~
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 06:55:01 PST
The patch is not invert the F and B colors when if the author set ::-moz-selection.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 07:00:07 PST
Sorry the patch is not fixed bug 176605.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2004-11-05 08:20:35 PST
I'm not going to have a chance to look at this until sometime next week at best.
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 22:59:11 PST
Created attachment 164816 [details] [diff] [review]
Patch rv2

I recreate the patch.
The new patch is adding "Briteness Difference".
I refered W3C document that is "Techniques For Accessibility Evalutation And
Repair Tools".
http://www.w3.org/TR/AERT#color-contrast

> Color visibility can be determined according to the following algorithm:
> 
> (This is a suggested algorithm that is still open to change.)
> 
> Two colors provide good color visibility if the brightness difference and the
color difference between the two colors are greater than a set range.
> 
> Color brightness is determined by the following formula:
> ((Red value X 299) + (Green value X 587) + (Blue value X 114)) / 1000
> Note: This algorithm is taken from a formula for converting RGB values to YIQ
values. This brightness value gives a perceived brightness for a color.
> 
> Color difference is determined by the following formula:
> (maximum (Red value 1, Red value 2) - minimum (Red value 1, Red value 2)) +
(maximum (Green value 1, Green value 2) - minimum (Green value 1, Green value
2)) + (maximum (Blue value 1, Blue value 2) - minimum (Blue value 1, Blue value
2))
> 
> The rage for color brightness difference is 125. The range for color
difference is 500.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-05 23:18:15 PST
Created attachment 164817 [details]
testcase
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-06 01:11:34 PST
testbuild.
http://www.d-toybox.com/mozilla/testbuilds/bug2909.zip
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-06 01:27:39 PST
Created attachment 164822 [details] [diff] [review]
Patch rv2.1
Comment 29 Justin Wood (:Callek) 2004-11-11 22:09:50 PST
Hi, I am the author of the patch on bug 234102 ;-)

I have a few minor nits, if this is to be added in, I would argue that you
extend the CSS Parser for background-color and color to include
|-moz-default-selection| or some such, which will query the Theme code (as the
Iterator Init calls[ed] before my patch)

in which case you can then use your color flipping for cases where the selection
color is not specified by a user on a website/application.  There may well be
perfectly good reasons to _not_ want color flipping, a person may want all text
to be same color and background change, all background same color (transparant
perhaps) and text change, or no color change at all.

For the last possibility at least see Ian's adhoc testcase for ::-moz-selection
at: http://www.hixie.ch/tests/adhoc/css/selectors/selection/mozilla/003.xml

If you agree with me on that point, please remove the r? and sr? to save the
developers time, though if you do not know how to code in the keyword stuff I am
willing to do it for you, or see you on IRC and try to walk you through it (as
best as I know it).

If you also wish I can try and finish up this bug for you with my idea in mind
(though I have no eta on that if it is/was your desire) and I would keep my
patch in my tree, and merge them both, that way there will be no need for either
of us to re-attach a patch once one of ours goes in to tree.

To end (a lengthy comment) I like your work on this so far, good job.  Though I
only agree with it on the "default" case.
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-11 23:09:41 PST
Justin:

Sorry, I cannot understand your comment(especially paragraph 2 and 3).
Because my English ability is very poor.

My patch was attached after your patch have been attached.
Therefore I think that your patch should be preferred.

But my patch need to know that the 'background-color' and 'color' are default or
not. Because if 'backround-color' or 'color' is set by author, this patch should
not work. If you set the properties in ua.css, I cannot be to know that the
properties are set by author...
Comment 31 Justin Wood (:Callek) 2004-11-12 20:37:00 PST
I will simplify my english for you.

My personal idea for fixing this is to create a new CSS [Cascading StyleSheet]
keyword to use on the |color| and |background-color| properties, (for ua.css)
which would tell Mozilla that the author has not specified a selection.

if you do not know how to add a keyword for this property, I can do the work, I
just do not know when (exactly) it will be done.

It is not up to me to decide who's patch will be done first, bz's (and roc's)
review time decides that.
Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-13 00:21:02 PST
Comment on attachment 164822 [details] [diff] [review]
Patch rv2.1

Thank you for re-comment.

I agree to create new keyword.
But I don't know how to add a keyword.
I want to leave it to you very much.
Comment 33 Justin Wood (:Callek) 2004-11-13 00:25:28 PST
No problem 
--> Taking

When I attach a new patch, may I receive an (unofficial) r+ from you (mjudge)
even though you are not a peer of the code, just for ease of my mind ;-)
Comment 34 Jungshik Shin 2004-11-13 01:31:42 PST
Justin, the email address ending with 'former-netscape.tld' is not valid any
more so that you need to find an alternative way to contact mjudge. However, I
don't think you have to bother to.
Comment 35 Justin Wood (:Callek) 2004-11-24 12:50:06 PST
Masayuki,

  I have decided that I will not take any credit for your work on this bug, I
will make a patch to add in the keywords, and change ua.css.

  You may then update your patch with needed changes. (If you agree)
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2004-11-24 19:26:59 PST
O.K. no problem.
Comment 37 Justin Wood (:Callek) 2005-01-29 21:35:56 PST
Ok this has taken me MUCH too long to get to (unexpectedly)

I suggest you try and get this in for 1.8b and not wait on me or my patches (iow
do not worry about CSS stuff, and assume any presence of ::-moz-selection is
still individual page done)

I'll worry about these issues in my other Bug as referenced here.

Sorry for the delay and the length of time it has taken me to get to this :(

1.8Beta freeze's on Febuary 9'th see:
http://www.mozilla.org/roadmap/branching-2005-01-25.png
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-01-30 06:05:06 PST
Created attachment 172879 [details]
another testcase
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-01-30 06:11:05 PST
*** Bug 140580 has been marked as a duplicate of this bug. ***
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-01-30 11:38:33 PST
Umm...

I found some problem.
It may not be in time for 1.8b.
However, I do my best. Please wait.
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-01-30 21:44:07 PST
Note:

If this bug will be fixed, on MacOS X, you may look reversed selection color
on many pages(it is including most popular color combination that is black text
and white background). But it is not bug. It is right. Because on MacOS X, white
and default selection background color does not have sufficient contrast.
For example, see following page.
http://www.nmt.ne.jp/~wotaku/index.html
On this page, the black text and light-blue background has sufficient contrast.
Therefore, this color combination has no problem.
But this background color and MacOS X's default selection background color does
not have sufficient contrast. So, if the page has black text, MacOS X's default
selection color is not suitable.

Why?
The reason is simple.

The canvas of web browser is different from widget.
On widget, the text color and background color are fixed always.
But these colors are flexible on the canvas of web browser.
If these colors are fixed always, such a patch is unnecessary.

If MacOS X users hate this patch, they can disabled this patch with using user
stylesheet.
*::-moz-selection{
  color: HighlightText;
  background-color: Hightlight;
}
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-01 06:08:03 PST
Created attachment 173076 [details] [diff] [review]
Patch rv3.0(work in progress)
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-01 07:54:49 PST
Created attachment 173082 [details] [diff] [review]
Patch rv4.0

For detail information of this patch, see comment in the patch.
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2005-02-01 10:08:20 PST
Comment on attachment 173082 [details] [diff] [review]
Patch rv4.0

>Index: layout/generic/nsTextFrame.cpp
>+  PRBool GetSelectionColors(nscolor *aForeColor, nscolor *aBackColor);

Please document what this function does (what the arguments are, what the
return value means, etc).

>+// The document said color difference is 500 and brightness differnce is 125000.

"difference".

Where are these differences of 500 and 125000?	What units are those in?

>+// This algorithm use brightness differnce only.

"difference".

>+// Because it is close to actual feeling.

I'm not sure what this means....

>+#define SUFFICIENT_BRIGHTNESS_DIFFERENCE 125000
>+#define BRIGHTNESS(color) (NS_GET_R(color) * 299 + NS_GET_G(color) * 587 + NS_GET_B(color) * 114)
>+#define BRIGHTNESS_DIFFERENCE(colorA, colorB) PR_ABS(BRIGHTNESS(colorA) - BRIGHTNESS(colorB))

Wouldn't some of this belong in nsColor more?  Or in nsCSSColorUtils?  Note
that the latter already has an NS_GetBrightness that seems very close to your
BRIGHTNESS macro, though returning values in a different range (scaled by a few
orders of magnitude).

Also, the fact that this will by default break selections on the Mac is
unacceptable.  Please fix that as needed (either by adding Mac-specific rules
to relevant CSS files, or by changing the selection code; talk to Mac people to
see which is better from their point of view).

Not reviewing the copy/paste of the logic for now, till these issues are
addressed.
Comment 45 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-02-01 10:19:43 PST
Boris, i haven't yet looked at his patch... In what way it break selections on
mac; wrong color or something else?
Comment 46 Boris Zbarsky [:bz] (still a bit busy) 2005-02-01 10:22:41 PST
See comment 41.
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-02 11:14:35 PST
Created attachment 173195 [details] [diff] [review]
Patch rv5.0

I created new patch.
In this patch, the color exchanging is not occured when we need to exchange if
black text is selected. And when NS_DONT_CHANGE_COLOR has been set to default
selection text color, too.
In other words, the color exchanging is occured when the default selection
color is lighter than 50% gray.

And I changed the exchanging algorithm, to simple one.
If the selected text color and default selection color are similar,
we can guess that the selected element's background color and default selection
color may be similar. Therefore, we are exchange the selection colors.
Otherwise, we don't need to exchange the colors.

WinIE is used selection background color for the exchanging algorithm. But I'm
not using it.
Comment 48 Justin Wood (:Callek) 2005-02-02 22:28:37 PST
Comment on attachment 173195 [details] [diff] [review]
Patch rv5.0

>+  // Get selection foreground and background colors by parameters.
>+  // The return value is these colors are exchanged by the text color.
>+  // If the return value is PR_TRUE, the colors are exchanged.
>+  PRBool GetSelectionColors(nscolor *aForeColor, nscolor *aBackColor);

how about "Return value is true if the foreground and background colors were
exchanged with each other, false if not."


>+//Find color based on mTypes[mCurrentIdx];
>+#define IS_SELECTION (mTypes? (mTypes[mCurrentIdx] & nsISelectionController::SELECTION_NORMAL) : (mCurrentIdx == (PRUint32)mDetails->mStart))

Lets change that comment, or remove it all-together, it is wrong here (imo)

>+  if (!IS_SELECTION) {
>+    return mOldStyle.mColor->mColor;
>+  // Using style rules of pseudo stylesheets.
>+  } else if (mSelectionPseudoStyle &&
>+             mSelectionStatus == nsISelectionController::SELECTION_ON) {
>+    return mSelectionPseudoFGcolor;
>   }

nit, no else after return.

>+  if (!IS_SELECTION) {
>+    return PR_FALSE;
>+  // Using style rules of pseudo stylesheets.
>+  } else if (mSelectionPseudoStyle &&
>+             mSelectionStatus == nsISelectionController::SELECTION_ON) {
>+    aColor = mSelectionPseudoBGcolor;
>+    *aIsTransparent = mSelectionPseudoBGIsTransparent;
>     return PR_TRUE;
>   }

same.

>+  GetSelectionColors(&foreColor, &aColor);
x2

Should we be caching this data in any-way...? (bz?)
...we make at least two calls to this on each paint with a selection currently
with your code. (GetBackgroundColor, GetForegroundColor

I am not a reviewer technically, but just the comments I found by peeking at
this...mostly nit's which I am sure bz will have similar ones.
Comment 49 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-03 03:38:22 PST
Created attachment 173248 [details] [diff] [review]
Patch rv6.0
Comment 50 Justin Wood (:Callek) 2005-02-03 07:14:30 PST
Comment on attachment 173248 [details] [diff] [review]
Patch rv6.0

>+  // Get selection foreground and background colors by parameters.
>+  // Return value is true if current color is selection colors, false if not.
>+  PRBool CurrentSelectionColors(nscolor *aForeColor, nscolor *aBackColor, PRBool *aBackIsTransparent);

Perhaps rename this function to GetSelectionColors since that does describe the
nature of this member function now, where before it was just swapping stuff.


>+  // We don't support selection color exchanging when selection text color is
>+  // NS_DONT_CHANGE_COLOR. Because the text color should not be background color.
>+  if (dontChangeTextColor)

nit: move this (comment) up before the declaration of dontChangeTextColor.

Other than that looks good to me, I suggest leaving this patch up to wait for
bz's review, unless he requests a new patch first.

Again, not a real reviewer, so if bz has conflicting comments, follow him not
me ;-)
Comment 51 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-03 07:31:40 PST
Created attachment 173265 [details] [diff] [review]
Patch rv6.1
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2005-02-03 11:31:34 PST
Comment on attachment 173265 [details] [diff] [review]
Patch rv6.1

>Index: layout/base/nsCSSColorUtils.h
>+#define NS_SUFFICIENT_LUMINANCE_DIFFERENCE 125000
>+#define NS_GET_LUMINANCE(c) \
>+          (NS_GET_R(c) * 299 + NS_GET_G(c) * 587 + NS_GET_B(c) * 114)
>+#define NS_LUMINANCE_DIFFERENCE(a, b) \
>+          PR_ABS(NS_GET_LUMINANCE(a) - NS_GET_LUMINANCE(b))

I believe the term used in Mozilla code, generally, is "luminosity", not
"luminance".

Note that I think it would be worth it to not duplicate the code from
NS_GetBrightness here.	Perhaps we should have an NS_GetLuminance method that
both NS_GetBrightness and this code will use?

>Index: layout/generic/nsTextFrame.cpp
>+  // Get selection foreground and background colors by parameters.
>+  // Return value is true if current color is selection colors, false if not.

I'm not sure what this comment means... Specifically, what does "current color
is selection colors" means?  Does that mean that we're supposed to be painting
a selection?  What can the caller assume about the out params when false is
returned?

I'd recommend javadoc syntax for the documentation of the params and the return
value.

This should also document whether *aBackColor is valid when *aBackIsTransparent
is set to true....

>+PRBool
>+DrawSelectionIterator::GetSelectionColors(nscolor *aForeColor,
>+                                              nscolor *aBackColor,
>+                                              PRBool  *aBackIsTransparent)

Odd indentation.

>+  // If current is not selection.
>+  if (!(mTypes ? (mTypes[mCurrentIdx] & nsISelectionController::SELECTION_NORMAL) :
>+       (mCurrentIdx == (PRUint32)mDetails->mStart))) {

This condition is really hard to understand... I know you just copied it, but
still.	Perhaps something like:

PRBool isSelection = 
  (mTypes && (mTypes[mCurrentIdx] & nsISelectionController::SELECTION_NORMAL))
||
  mCurrentIdx == (PRUint32)mDetails->mStart;
if (!isSelection) {
  ....
}

>+  if (mSelectionPseudoStyle &&
>+      mSelectionStatus == nsISelectionController::SELECTION_ON) {

This changes the logic, doesn't it?  In the case when
mOldStyle.mSelectionTextColor is NS_DONT_CHANGE_COLOR, in particular, we will
now return a different foreground color.  Is there a reason for this?

>+  // We don't support selection color exchanging if selection is exchanged when the
>+  // text color is black.
[etc]

I'll try to make sense of this comment when I next look at this patch.	I think
it's just a matter of English fixup....

>+  // If the combination of the selection text color and the original text color are sufficient contrast,
>+  // probably these background colors are not alike. Therefore, we should not exchange selection colors.

Hmm... But wouldn't it make more sense to just compare the selection background
to our background?  I guess the problem is that we don't really have "our
background" here?

Would it make sense to call EnsureDifferentColors rather than reversing?  I'm
not sure what that does, so maybe not... check?

>+          PRBool     isSelection = iter.GetSelectionColors(&currentFGColor,
>+                                     &currentBKColor, &isCurrentBKColorTransparent);

Weird indent.

>+          PRBool     isSelection = iter.GetSelectionColors(&currentFGColor,
>+                                     &currentBKColor, &isCurrentBKColorTransparent);

And here.
Comment 53 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-03 12:20:37 PST
> I'd recommend javadoc syntax for the documentation of the params and the return
> value.

What's the "javadoc syntax"?
I don't know it.

>>+  if (mSelectionPseudoStyle &&
>>+      mSelectionStatus == nsISelectionController::SELECTION_ON) {
> 
> This changes the logic, doesn't it?  In the case when
> mOldStyle.mSelectionTextColor is NS_DONT_CHANGE_COLOR, in particular, we will
> now return a different foreground color.  Is there a reason for this?

Currentry behavior is a bug.
If a page author write following rules,

p{
  color: white;
}
p::-moz-selection{
  color: rgb(1, 1, 1);
}

should render the text with rgb(1, 1, 1). not white.
# currently, this pattern is rendered with white text.

> But wouldn't it make more sense to just compare the selection background
> to our background?

We cannot get autual background color. Because we have to get ancestor
background. But it is very hard work. And if author used background image, we
cannot get the image's color. Therefore, we must guess the actual background
color from text color. If there are some better idea, please tell me.

> Would it make sense to call EnsureDifferentColors rather than reversing?  I'm
> not sure what that does, so maybe not... check?

EnsureDifferentColor is not helpful when selection foreground color is not
NS_DONT_CHANGE_COLOR in many case(if it is not so, the user cannot read the
selection text in all applications). But I will combine the function and
GetSelectionColors in next patch.
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-04 02:03:06 PST
> Currentry behavior is a bug.
> If a page author write following rules,
> 
> p{
>   color: white;
> }
> p::-moz-selection{
>   color: rgb(1, 1, 1);
> }
> 
> should render the text with rgb(1, 1, 1). not white.
> # currently, this pattern is rendered with white text.

Sorry. It is wrong. Mozilla render correctly in this case. But Mozilla has
another bug.
If the default selection text color is rgb(1, 1, 1) or on MacOS X,
the color property in ::-moz-selection is always ignored. It is bug. It is not
related to a system default selection text color, it should be rendered by
specified color in ::-moz-selection.
Comment 55 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-04 03:43:46 PST
Created attachment 173354 [details] [diff] [review]
Patch rv6.2
Comment 56 Boris Zbarsky [:bz] (still a bit busy) 2005-02-04 22:28:25 PST
> What's the "javadoc syntax"?

http://java.sun.com/j2se/javadoc/writingdoccomments/index.html

> # currently, this pattern is rendered with white text.

Yes, I realize that.  But is that perhaps the desired behavior, on Mac?  ccing
Ian and Simon in case they can shed some light on this.  The question is, given
that selections on mac are styled with the foreground color, should the "color"
of ::selection apply to them?  I'm guessing yes, but we don't do it now, so I
assume there's a reason....
Comment 57 Simon Fraser 2005-02-09 21:29:07 PST
I believe long ago that we decided that Mac users didn't like it if you messed
with their selection color in any way.
Comment 58 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-15 00:30:55 PST
(In reply to comment #57)
> I believe long ago that we decided that Mac users didn't like it if you messed
> with their selection color in any way.

If Mac users doesn't like changing the selection color, I think that the
background color of selection should not be changed by author too.
Currently behavior is incompletely.
Comment 59 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-15 01:32:52 PST
Boris said that Boris cannot allow to exchange the default selection colors on
Mac when the page is black text and white background.
# It is for Mac only? or all platforms?
If using "Windows XP style" and using "silver" on WinXP, the default selection
colors are exchanged on WinIE when the page is black text and white background.
I think this behavior is better behavior for Web Browsers. Because the web pages
are not standard widget of OS.

I propose following.

1. If the selection text color is NS_DONT_CHANGE_COLOR, we are not exchanging
the selection colors always.

2. If the selection text color is NS_DONT_CHANGE_COLOR, we ignore
::-moz-selection pseudo-elements.

3. If the selection text color is not NS_DONT_CHANGE_COLOR, we always decide
that the selection color are exchanged the colors or not.
(It is including when the page has black text and white background.)
Comment 60 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-02-15 02:42:45 PST
Created attachment 174370 [details] [diff] [review]
Patch rv7.0
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 08:24:50 PST
Simon, the question is what we should do if the page explicitly styles the
selection color.  In that case, should we be changing it?  I would think the
answer is yes.....
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 22:41:56 PST
Comment on attachment 174370 [details] [diff] [review]
Patch rv7.0

Simon, what do you think?  Especially the NS_DONT_CHANGE_COLOR code... what
exactly should that be doing when ::selection is being used?
Comment 63 Simon Fraser 2005-03-22 21:29:55 PST
I tried the patch, and it seems reasonable. It seems correct to either mess with
both the foreground and background of the selection, or neither but not just one.
Comment 64 Boris Zbarsky [:bz] (still a bit busy) 2005-04-03 11:58:28 PDT
Comment on attachment 174370 [details] [diff] [review]
Patch rv7.0

>Index: layout/base/nsCSSColorUtils.cpp

>-#define RED_LUMINOSITY        30
>-#define GREEN_LUMINOSITY      59
>-#define BLUE_LUMINOSITY       11

Please keep the defines (with new values) -- that makes code using those
numbers much clearer to read.

>Index: layout/generic/nsTextFrame.cpp

>+   * Get foreground color, background color, the background is transparent or not
>+   * and current range is selection or not.

Maybe:

"Get foreground color, background color, whether the background is transparent,
and whether the current range is the normal selection."


>+   * @param aForeColor           This is returned the foreground color of current range.

@param aForeColor [out] returns the foreground color of the current range.

>+   * @param aBackColor           This is returned the background color of current range.

@param aBackColor [out] returns the background color of the current range.

>+   *                               Note that this value is unspecified if aBackIsTransparent is true

I think "undefined" is better than "unspecified".

>+   * @param aBackIsTransparent  This is returned that the background is transparent or not.

@param aBackIsTransparent [out] returns whether the background is transparent.

>+   * @return                      If true, current range is selection. Otherwise, it isn't so.

@return whether the current range is a normal selection

>+DrawSelectionIterator::GetSelectionColors(nscolor *aForeColor,

>+  PRBool isSelection = (mTypes && mTypes[mCurrentIdx] & nsISelectionController::SELECTION_NORMAL) ||

For clarity, could you put parens around the bitwise and expression?

>+  if (NS_LUMINOSITY_DIFFERENCE(*aForeColor, mOldStyle.mColor->mColor) >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE)

Please line-break as needed so this doesn't go over the 80th column?

>           else
>             newWidth =0;
>-          
>+

Please take out this whitespace change.

With those nits, sr=bzbarsky.  If you get a chance to update this before the
freeze (that's in about 58 hours), I can check it in for you; if you haven't
gotten to it by sometime Tuesday night Pacific I'll probably try to update the
patch myself and check it in.  I think we definitely want this for 1.8, in any
case.

Thank you very much for your patience, and my apologies that the reviews on
this took so long.... :(
Comment 65 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-03 13:16:09 PDT
Created attachment 179497 [details] [diff] [review]
Patch for check-in
Comment 66 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-03 13:40:56 PDT
I will sleep soon.(Now is 6:00 AM at JST)
Therefore the patch will be checked-in after I get up and new patch's build test.
Thanks.
Comment 67 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-04 03:49:41 PDT
checked-in.

-> FIXED

Note that this patch is not enough for all cases.
But if you hope that you reopen this bug, you should propose better patch or
better idea.
Comment 68 Boris Zbarsky [:bz] (still a bit busy) 2005-04-04 07:22:46 PDT
Actually, if you have a better idea file a _new_ bug.
Comment 69 Greg Campbell 2005-04-05 11:02:41 PDT
Since this was checked in, highlighting any black text on white background
(including the Location bar) makes gray text on a black background. IE and NN4
and Fx pre-this-patch do black text with gray background in location bar.

Is the new behavior desired, or is it a bug introduced by this patch? FWIW, it
seems visually less attractive, inconsistent with other Windows apps, and no
more readable.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050404
Firefox/1.0+
Comment 70 Boris Zbarsky [:bz] (still a bit busy) 2005-04-05 11:11:16 PDT
That sounds like a consequence of us comparing foreground colors rather than
background colors.... I guess the special-case hack for the Mac isn't good
enough, since Windows actually does specify black as the selected text color.

If this is a common selection behavior (and sounds like it is, since both
Windows and Mac do it), then it sounds like we just can't use this approach at all.
Comment 71 Charles Fenwick 2005-04-05 14:21:48 PDT
See bug 289181...
Comment 72 Boris Zbarsky [:bz] (still a bit busy) 2005-04-05 15:51:48 PDT
Er.. Charles, what does that cache service bug have to do with this bug?
Comment 73 Charles Fenwick 2005-04-05 19:50:39 PDT
Re question posed in previous comment:

Absolutely nothing, I looked at the wrong tab when checking my reference.

The bug I intended to cite was bug 289191.  Having read this bug in its
entirety, though, I understand that the newly reported bug is referring to what
is now intended behavior.
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2005-04-05 20:05:26 PDT
OK, yeah.  Bug 289191 shows another case when the algorithm this patch used fails...

At this point, I think we should probably back this patch out and try to come up
with a better idea.
Comment 75 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-05 20:35:57 PDT
The behavior of bug 289191 is in my designed.
I think this problem should be solved by web page author.
Because the combination of white background and light foregrond color is not
having sufficient contrast. See http://www.w3.org/TR/AERT#color-contrast
I think that if there is a problem in non-sufficient contrast page, we should
not have a problem in sufficient contrast page.
Comment 76 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-05 20:48:10 PDT
I hope that if the patch is back-out, I don't want to do.
In that time, I want to create new patch that is disabled the reverse algorithm.
Because when I will retry to create the patch, I hope that the patch size is small.

i.e.,

   // We don't support selection color exchanging when selection text color is
   // NS_DONT_CHANGE_COLOR. Because the text color should not be background color.
-   if (dontChangeTextColor) {
+   if (dontChangeTextColor)
     *aForeColor = EnsureDifferentColors(*aForeColor, *aBackColor);
-    return PR_TRUE;
-  }
 
+  return PR_TRUE;
+#if 0
   // Note: We assume that the combination of the background color and text color

Comment 77 Boris Zbarsky [:bz] (still a bit busy) 2005-04-05 21:03:08 PDT
Sure, if we decide to "back this out" that's what we should do.  The code
cleanup that happened here is good.

The problem with bug 289191 is that the selection actually shows up _less_,
since its background color is identical to the page background there...  Perhaps
we need to test for closer luminosity match before reversing?

Also, none of that handles the Windows default selection issue, which is a much
bigger problem.
Comment 78 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-05 21:27:12 PDT
I have another idea.
We get the background color at *ancestor* block level element.
And we test between this color and selection-background color.

In this idea have two problem.
1. performance is worse than current patch.
2. If the page has background-image, it may fail.
Comment 79 Boris Zbarsky [:bz] (still a bit busy) 2005-04-05 23:32:16 PDT
We could also just find the closest non-transparent background in general... I
believe we have utility functions for this, no?
Comment 80 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 05:49:00 PDT
Created attachment 179838 [details] [diff] [review]
Patch for disable previous patch
Comment 81 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 07:35:22 PDT
Created attachment 179846 [details]
testcase
Comment 82 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 07:57:00 PDT
Created attachment 179849 [details]
testcase2
Comment 83 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 09:54:07 PDT
Comment on attachment 179838 [details] [diff] [review]
Patch for disable previous patch

O.K.
We should not back out previous patch.
I will attach new patch with advanced algorithm.
Comment 84 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 10:49:20 PDT
Created attachment 179869 [details] [diff] [review]
Patch rv8.0
Comment 85 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 10:52:04 PDT
This patch fixes bug 289191 and bug 176605.
Comment 86 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 10:58:54 PDT
This patch assume that if web page author uses 'background-image', the author
_always_ specify the 'background-color' that value is similar to the image.

So following page doesn't work correctly. Because this page specified
'background-color: #ffffff'.
http://www.samurai-zero.jp/
Comment 87 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 11:09:32 PDT
Created attachment 179872 [details] [diff] [review]
Patch rv8.1

Oops..
I have a mistake.
Comment 88 Anne (:annevk) 2005-04-06 11:21:14 PDT
If for some element 'background-color' computes to 'transparent' will the
nearest ancestor background color be chosen? If not this is not the right
approach I think.
Comment 89 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 11:25:51 PDT
I request blocking1.8b2.

The left work is to fix the regression of previous patch.
I think that this is very important UI issue.
I think that many people should test the new patch in 1.8b2 and firefox1.1a.
Comment 90 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-06 11:28:19 PDT
(In reply to comment #88)
> If for some element 'background-color' computes to 'transparent' will the
> nearest ancestor background color be chosen? If not this is not the right
> approach I think.

Yes. You are right.
I'm using |nsCSSRendering::FindNonTransparentBackground| that find nearest
ancestor(this is include current element) background color.
Comment 91 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 00:53:05 PDT
Comment on attachment 179838 [details] [diff] [review]
Patch for disable previous patch

Boris:

If you don't allow the new patch(attachment 179872 [details] [diff] [review]), you should grant the
attachment 179838 [details] [diff] [review] that is disable reverse selection colors.(But it is not
disabled ::-moz-selection changes on Mac)
Comment 92 Boris Zbarsky [:bz] (still a bit busy) 2005-04-07 10:05:10 PDT
Comment on attachment 179872 [details] [diff] [review]
Patch rv8.1

>Index: layout/generic/nsTextFrame.cpp

>+    const nsStyleBackground* bg =
>+      nsCSSRendering::FindNonTransparentBackground(aStyleContext);
>+    NS_ASSERTION(bg, "Cannot find NonTransparentBackground.");
>+    if (bg)

Either assert or check, not both.  Given existing code that uses this function,
I think the assert is reasonable.

>+  // Otherwise, if the combination of selection foreground color and frame
>+  // background color is sufficient contrast. Should exchange the 

"sufficient contrast, should"  (replace full stop with comma).

>+  if (foreLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE) {

...

>+  if (backLuminosityDifference < foreLuminosityDifference) {

You only get into these if statements if backLuminosityDifference is less than
the sufficient difference.  So the second case includes the first one, no?  In
other words, if you plan to switch if the foreground would show up better, it
doesn't matter whether the foreground shows up "enough"...  So you can remove
that first if check
Comment 93 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 10:47:21 PDT
>>+  if (foreLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE) {
> 
> ...
> 
>>+  if (backLuminosityDifference < foreLuminosityDifference) {
> 
> You only get into these if statements if backLuminosityDifference is less than
> the sufficient difference.  So the second case includes the first one, no?  In
> other words, if you plan to switch if the foreground would show up better, it
> doesn't matter whether the foreground shows up "enough"...  So you can remove
> that first if check

No. Atcually, I remove the first if statement, the result is not good.
Comment 94 Boris Zbarsky [:bz] (still a bit busy) 2005-04-07 10:56:10 PDT
Why not?  That makes no sense.  You have the following logic in that patch:

if (backLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE)
  return;

if (foreLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE) {
  reverse;
  return;
}

if (backLuminosityDifference < foreLuminosityDifference ) {
  reverse;
  return;
}

I'm saying the entire middle block can be eliminated.  Indeed, say the middle
block is entered. That means that we have:

1)  backLuminosityDifference < NS_SUFFICIENT_LUMINOSITY_DIFFERENCE
2)  foreLuminosityDifference >= NS_SUFFICIENT_LUMINOSITY_DIFFERENCE

But in that case the third block would be entered if the middle block were not
there.
Comment 95 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 11:02:24 PDT
Oops..

You're right.
I missed.
Comment 96 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 11:03:46 PDT
Created attachment 179964 [details] [diff] [review]
Patch rv8.2
Comment 97 Boris Zbarsky [:bz] (still a bit busy) 2005-04-07 11:10:26 PDT
Comment on attachment 179964 [details] [diff] [review]
Patch rv8.2

>Index: layout/generic/nsTextFrame.cpp
>+  // If the combination of selection background color and frame background color
>+  // is sufficient contrast. Don't exchange the selection colors.

Please change this comment as I requested.

>+  // Otherwise, we should use the larger color of contrast for the selection
>+  // background color.

"we should use the higher-contrast color for the selection background color"

With those fixed, r=bzbarsky.

I'd really like David to look at this, though...
Comment 98 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 11:14:09 PDT
Created attachment 179966 [details] [diff] [review]
Patch for check-in
Comment 99 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-07 11:16:33 PDT
Created attachment 179967 [details] [diff] [review]
Patch for check-in
Comment 100 Daniel Höpfl 2005-04-08 09:30:23 PDT
I think the algorithm used now misses an important point: Users opinion.

Let me explain that: I have configured a bright grey (f0f0f0) as default
background. My selections background is some kind of dirty yellow (ead074),
selected text is black.

When will the current algorithm revers my settings? On every page that uses a
bright (e.g. white) background.

When should it revers my settings? On every page that uses a yellowish background.

I think the best solution would be not only to look at the luminosity of the
colors but also at the color (there are reds and greens that have the same
luminosity still a red area on a green background is clearly visible).

A simpler solution could be not to be stuck with W3C but to trust in the user.
W3C says that the colors should differ by 50% of the luminosity's maximum
difference. The user (me) says (ead074 to f0f0f0 - 13.6%) are different enough.

What about a patch that does what I tried to explain: the sufficient luminosity
difference is no longer fixed to 50% but to the lowest of 

- 50% 
- NS_LUMINOSITY_DIFFERENCE(defaultBackground, selectionBackground
- ( NS_LUMINOSITY_DIFFERENCE(selectionForeground, selectionBackground) )

Just an idea.
Comment 101 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 09:44:19 PDT
(In reply to comment #100)

I think that the idea of previous comment is nice idea.
Thanks. Please wait. I'm testing new patch.
Comment 102 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 10:20:50 PDT
Created attachment 180069 [details] [diff] [review]
Patch rv9.0

This patch looks good for me better than rv8.2.
Comment 103 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 10:27:28 PDT
Comment on attachment 180069 [details] [diff] [review]
Patch rv9.0

Sorry for the spam.
I have an advanced idea that fixes bug 289287.
Please wait.
Comment 104 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 11:06:38 PDT
Created attachment 180079 [details] [diff] [review]
Patch rv10.0
Comment 105 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 22:31:03 PDT
If the patch is checked-in, the fixed bug list is this, bug 176605, bug 289191
and bug 289287.
Comment 106 Boris Zbarsky [:bz] (still a bit busy) 2005-04-08 22:50:12 PDT
So let me see if I understand this...  That patch assumes that "sufficient
contrast" is a luminosity difference that is at least as big as that between
selection color and selection background or selection background and default
background, right?

That has the same problem as pointed out in comment 100.  Basically, the issue
is that one can have colors of identical luminosity that look quite different
(contrasting) to the human eye.  Consider as a simple example the following:

rgb(255, 30, 0);
rgb(59, 130, 0);
Comment 107 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 23:04:12 PDT
(In reply to comment #106)
> So let me see if I understand this...  That patch assumes that "sufficient
> contrast" is a luminosity difference that is at least as big as that between
> selection color and selection background or selection background and default
> background, right?
"sufficient contrast" is minimum value of the contrast between selection color
and selection background or selection background and default background or 50%.

> That has the same problem as pointed out in comment 100.  Basically, the issue
> is that one can have colors of identical luminosity that look quite different
> (contrasting) to the human eye.  Consider as a simple example the following:
> 
> rgb(255, 30, 0);
> rgb(59, 130, 0);
If on the colorful device, you are right.
But if these colors displayed on monochrome display or color-blindness user
looks it, the colors are similar.

Comment 108 Boris Zbarsky [:bz] (still a bit busy) 2005-04-08 23:09:51 PDT
Sure.  But the point is, people are using such colors for their selections,
because they _do_ use color devices...
Comment 109 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 23:16:25 PDT
Created attachment 180148 [details]
On colorful device, monochrome device and if the user color-blindness(2 patterns)
Comment 110 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 23:23:12 PDT
(In reply to comment #108)
> Sure.  But the point is, people are using such colors for their selections,
> because they _do_ use color devices...

I don't think so. Becasue such like as... If the colors are having contrast for
human eyes but on monochrome or for color-blindness usrs don't having contrast,
the colors are complementary color. I think that many user don't use the
combination of complementary colors for adjoined colors.
Comment 111 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-08 23:42:25 PDT
Created attachment 180150 [details]
screen shot of comment 108

I cannot watch this screenshot while long time....
Comment 112 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 09:55:01 PDT
Boris:
We can use following algorithm.
In this case, the comment 100's problem is not occured.
But if we implement so, we don't support monochrome display.
(i.e., We cannot use Minimo on monochrome devices)

http://www.w3.org/TR/AERT#color-contrast
> Color difference is determined by the following formula:
> (maximum (Red value 1, Red value 2) - minimum (Red value 1, Red value 2)) +
(maximum (Green value 1, Green value 2) - minimum (Green value 1, Green value
2)) + (maximum (Blue value 1, Blue value 2) - minimum (Blue value 1, Blue value 2))
> 
> The rage for color brightness difference is 125. The range for color
difference is 500.
Comment 113 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 11:50:38 PDT
Created attachment 180300 [details] [diff] [review]
Patch rv11.0(test patch, I don't like this)
Comment 114 Boris Zbarsky [:bz] (still a bit busy) 2005-04-10 11:55:02 PDT
Can we detect what sort of device we're running on, perhaps?  And expose that on
the prescontext or something?
Comment 115 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 14:32:02 PDT
Comment on attachment 180300 [details] [diff] [review]
Patch rv11.0(test patch, I don't like this)

This algorithm doesn't work fine.
This algorithm have comment 100's problem too.
Comment 116 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 14:40:05 PDT
I think that currently, the best algorithm is using luminosity(rv10.0).
If we can fix the comment 100's problem, we may use HSV color space.
But this algorithm is difficult for me.
if S = 0 and V = 0, if H differ 30, the colors are not similar.
But S and V is not 0, I don't know relevant value...
Comment 117 Ivan Ičin 2005-04-10 14:47:09 PDT
I hope that this is not bug spam. If it is not helpful, just ignore it.

Three main color components (RGB)doesn't have the same importance for the eye.
If you want to make the grayscale the way eye see it, you should use this
approximation: Gray= 60%green + 30%red + 10% blue.

The values are not exact, but if it helps it is not impossible to find exact
values on the internet.
Comment 118 Boris Zbarsky [:bz] (still a bit busy) 2005-04-10 14:52:36 PDT
Ivan, please do take a look at the patch before making code suggestions....
Comment 119 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 14:58:53 PDT
(In reply to comment #117)
> I hope that this is not bug spam. If it is not helpful, just ignore it.
> 
> Three main color components (RGB)doesn't have the same importance for the eye.
> If you want to make the grayscale the way eye see it, you should use this
> approximation: Gray= 60%green + 30%red + 10% blue.
> 
> The values are not exact, but if it helps it is not impossible to find exact
> values on the internet.

Thanks Ivan. But the luminosity caluculation is using in patch rv10.0.
We are troubled the calculations weak point.
If we can fix the weak point, we must use HSV color space. Because the color
space is most similar as human eyes.

# I will sleep soon... I retry to create new patch after wake up.
Comment 120 Boris Zbarsky [:bz] (still a bit busy) 2005-04-10 16:42:45 PDT
Comment on attachment 180079 [details] [diff] [review]
Patch rv10.0

Thought about this some more, and r=bzbarsky.  This is a good bit better than
what we had now, and 1.8b is getting kinda close...
Comment 121 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-10 22:54:28 PDT
Thank you, Boris.
I will create new patch in _new bug_ after this bug fixed...
Comment 122 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-18 00:45:24 PDT
I have been sent mail to dbaron. But he doesn't superreview yet...
Boris, can you superreview it?
Comment 123 Boris Zbarsky [:bz] (still a bit busy) 2005-04-18 22:38:39 PDT
Comment on attachment 180079 [details] [diff] [review]
Patch rv10.0

David's not going to get to this for a bit, apparently... Let's get this landed
so it gets some testing.

Requesting 1.8b2 approval; we need something like this to not break default
selection behavior on Windows too badly.
Comment 124 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-18 23:06:50 PDT
Checked-in to Trunk.
Thanks.


*Note that don't reopen this bug.*
Comment 125 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-19 01:51:04 PDT
I opened bug 290919 that is advanced algorithm for color device.
Comment 126 Aaron Leventhal 2005-04-19 05:35:35 PDT
Related: bug 111526 (outline color)
Comment 127 Ben Ruppel 2005-04-19 14:40:39 PDT
Hey, I tried out the following link with the 20050419 trunk build on win2k:
https://infosec.navy.mil/

I hope you can load it (it is a .mil page).  It looks like there are two side
sections with navy blue backgrounds and a middle section with a white
background.  The white background might be an image.  Selecting text in the
middle part of the page results in no visual result for normal black text, and
turning the blue hyperlinks black.  The background color stays white, which is
not visible on a white background.

I can post the page source if Masayuki can not view it.
Comment 128 Greg Campbell 2005-04-19 14:53:51 PDT
(In reply to comment #127)
> https://infosec.navy.mil/

All looks good to me:
(text/background --> selected)
black/white --> black/gray
blue(link)/white --> black/gray (but underline is still blue)
yellow/navy --> black/gray

Comment 129 Ben Ruppel 2005-04-19 17:40:01 PDT
What build are you running?  Is it trunk?  What OS?
Comment 130 Ben Ruppel 2005-04-19 19:19:36 PDT
Hmm, works fine on home system, which is winxp.  Will double check build and try
new profile on my win2k system tomorrow.
Comment 131 Boris Zbarsky [:bz] (still a bit busy) 2005-04-19 21:09:29 PDT
> The white background might be an image.

It is.  With images disabled, the page has black text on a navy blue background,
and is utterly unreadable... This patch had sort of assumed (and this is a big
assumption) that people who use background images would also set a reasonable
background color... :(
Comment 132 Greg Campbell 2005-04-20 00:25:40 PDT
(In reply to comment #129)
> What build are you running?  Is it trunk?  What OS?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050419
Firefox/1.0+
Comment 133 Ben Ruppel 2005-04-20 05:13:35 PDT
Boris, 
So is the behavior from comment #127 the expected behavior?  Do you see it
yourself?  When I run the April 19 trunk build on my winXP machine I see the
behavior from comment #128 (I think this is just what firefox did before the
patch).  I tried a new profile and I still see the behaior from comment #128 on
winXP.  Anyone else able to do a side-by-side comparison of win2k with winxp and
this bug?  Please excuse me if I'm being an idiot. 

Also, I hate to say it but IE somehow figures out what background to use where
on the page I noted in comment 127.
Comment 134 Boris Zbarsky [:bz] (still a bit busy) 2005-04-20 08:34:37 PDT
> So is the behavior from comment #127 the expected behavior?  Do you see it
> yourself?

The exact behavior will depend on your default selection colors...  Mine are
white-on-medium-blue, so when I select the black text I get medium-blue-on-white
(so the selection background is invisible) and when I select the blue text I get
just a slight darkening of the blue text.  It sounds like your default selection
colors are white on black or black on white, which would explain the behavior
you're seeing.

As for "expected", do you mean "given this patch", or in general?
Comment 135 Ben Ruppel 2005-04-20 09:43:11 PDT
I meant given this patch.  I thought I had come up with some malfunction in the
way it was working.  Thanks for explaining the dependence on user selection
color settings to me.
Comment 136 rbs 2005-04-20 19:12:13 PDT
Created attachment 181374 [details]
screenshot: comparison of the selection before&after the patch

Unfortunately the patch has made the selection basically unreadable (at least
on Win2K), especially for those of us like myself who have poor eye sight.
Comment 137 rbs 2005-04-20 19:13:43 PDT
s/unreadable/undistinguishable/
Comment 138 Boris Zbarsky [:bz] (still a bit busy) 2005-04-20 19:47:31 PDT
rbs, what build is that?  That's not what I see with a 2004-04-19 Linux build...
But again, the exact behavior is selection-color-dependent.
Comment 139 rbs 2005-04-20 20:10:25 PDT
Yesterday build. I was expecting the default behavior (or something similar)
that other apps are showing. I have a vanilla system where the selection
settings haven't been tampered with. They are still as shipped with the OS. I am
rebuilding now and will let you know if things get better.
Comment 140 Ben Ruppel 2005-04-20 20:14:07 PDT
I have confirmed that I can reproduce the unreadable behavor on my example web
page by changing my selection colors to black/white.  When I leave them as
silver/black (the defaults for winxp's silver scheme) the page is readable.

I think the way this is turning out is a problem.  The question is: 
is it possible to tweak the algorithm to avoid it, or will it always be a
problem because mozilla can not "figure out" that it needs to do something about
the white background image?
Comment 141 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-21 01:30:19 PDT
Created attachment 181395 [details]
screenshot

rbs:
I cannot reproduce it.
My system is Win2k(default selection colors).
Comment 142 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-04-21 01:43:34 PDT
Ben Ruppel:

The web page author should spcify the background-color when using
background-image. It is "general" manners. See CSS2.1 specification.
http://www.w3.org/TR/CSS21/colors.html#background-properties
> When setting a background image, authors should also specify a background
color that will be used when the image is unavailable. 

But if web page users have the problem, we can inhibit the reverse behavior.
If you want to inhibit the reverse selection colors, you should add following
style in your user style sheet.

::-moz-selection{
  background-color: Highlight;
  color: HighlightText
}
Comment 143 rbs 2005-04-21 01:58:16 PDT
My clobber-all build-all has completed. And yes, the problem is gone (on my
vanilla Win2K-English). I get what the 'before' used to be on attachment 181374 [details].
Comment 144 pete 2005-04-30 16:35:20 PDT
I use white text on orange bg as my system selection color and IE's algorithm
and now mozilla's algorithm annoys me to no end. It's very discerning as a user
to see an alternate selection color used (not to mention it's pretty unreadable
seeing orange text on a white bg). I delibritly set my colors this way because I
do not visit pages that this kind of selection bg-color would have a chance of
blending against. I suggest either a pref that lets me disable it or a pref that
lets me set the tolerance value such that there is a value that always chooses
my selection colors no matter what instead of hardcoding it.
Comment 145 Akkana Peck 2005-06-27 20:43:06 PDT
Another problem with the new algorithm: it may choose a selection background
that contrasts with the page background, but it doesn't choose a selection text
color that contrasts with the selection background. Since this change I'm seeing
a lot of white text on a sort of dingy mustard yellow background. The colors are
both quite light and the text isn't readable. For instance, try it on a
background color like #E8FFF8.
Comment 146 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2005-06-27 20:54:20 PDT
Note:

If you have a problem, please file a new bug with your selection color,
selection background-color and page background-color.

DON'T ADD THE COMMENT HERE.
Comment 147 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-05-01 18:40:24 PDT
*** Bug 208693 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.