Closed Bug 56314 Opened 24 years ago Closed 19 years ago

reverse selection colors when page background is similar to default selection background

Categories

(Core :: DOM: Selection, defect, P1)

Product:
Core
Component:
DOM: Selection
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta2

People

(Reporter: sgifford+mozilla-old, Assigned: masayuki)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: Don't add comment for new problem. See #146.)

Attachments

(10 files, 19 obsolete files)

Patch rv7.0
17.17 KB, patch
sfraser_bugs
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch for check-in
17.07 KB, patch
Details | Diff | Splinter Review
Patch for disable previous patch
1.80 KB, patch
Details | Diff | Splinter Review
testcase
257.11 KB, text/html
Details
testcase2
276.60 KB, text/html
Details
Patch rv10.0
7.51 KB, patch
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
Details
screen shot of comment 108
285.77 KB, image/jpeg
Details
screenshot: comparison of the selection before&after the patch
31.51 KB, image/png
Details
screenshot
87.28 KB, image/jpeg
Details 
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
hmm..I don't know of one. Looking...
Not going to get to this by RTM, future.
Target Milestone: --- → Future
changing selection qa to tpreston.
QA Contact: blaker → tpreston
*** Bug 91708 has been marked as a duplicate of this bug. ***
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).
Severity: enhancement → normal
Summary: Dynamic detection of ui.textSelectBackground → reverse selection colors when page background is similar to default selection background
Attached file testcase (obsolete) — 

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
4xp as per comment 6
Keywords: 4xp

    
  
Blocks: 176605

    
    

    
  
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.

    
    

    
  
*** Bug 184277 has been marked as a duplicate of this bug. ***
Blocks: 140580

    
    

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

    
    

    
  
See also bug 111526, "link focus outline (invert) invisible on gray backgrounds".

    
    

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

    
  
Blocks: 164421

    
    

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

    
    

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

    
  
Blocks: 255941

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (obsolete)
        — 
          — Splinter Review
      

    


  
I created the patch.
But it conflict with bug 234102.

    
    

    
  
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)
                                                         ~~~~

        Attachment #164736 -
        Flags: superreview?(roc)

        Attachment #164736 -
        Flags: review?(bzbarsky)

    
    

    
  
The patch is not invert the F and B colors when if the author set ::-moz-selection.

    
    

    
  
Sorry the patch is not fixed bug 176605.

    
    

    
  
I'm not going to have a chance to look at this until sometime next week at best.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv2 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #164736 -
        Attachment is obsolete: true

        Attachment #164816 -
        Flags: superreview?(roc)

        Attachment #164816 -
        Flags: review?(bzbarsky)

        Attachment #164736 -
        Flags: superreview?(roc)

        Attachment #164736 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached file
          
        testcase (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv2.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #164816 -
        Attachment is obsolete: true

        Attachment #164816 -
        Flags: superreview?(roc)

        Attachment #164816 -
        Flags: review?(bzbarsky)

        Attachment #164822 -
        Flags: superreview?(roc)

        Attachment #164822 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

    
    

    
  
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...

    
    

    
  
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 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.

        Attachment #164822 -
        Flags: superreview?(roc)

        Attachment #164822 -
        Flags: review?(bzbarsky)

    
    

    
  
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 ;-)
Assignee: mjudge → 116057

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
O.K. no problem.

    
    

    
  
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
Assignee: 116057 → masayuki
Blocks: 234102
No longer depends on: 234102
Target Milestone: Future → mozilla1.8beta

    
    

    
  

      
      
      
      

        Attached file
          
        another testcase (obsolete)
        — 
      

    


  

        Attachment #164822 -
        Attachment is obsolete: true

    
    

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

    
    

    
  
Umm...

I found some problem.
It may not be in time for 1.8b.
However, I do my best. Please wait.
Status: NEW → ASSIGNED

    
    

    
  
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;
}

        Attachment #173076 -
        Attachment description: Patch rv1.3(work in progress) → Patch rv3.0(work in progress)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv4.0 (obsolete)
        — 
          — Splinter Review
      

    


  
For detail information of this patch, see comment in the patch.

        Attachment #173076 -
        Attachment is obsolete: true

        Attachment #173082 -
        Flags: superreview?(bzbarsky)

        Attachment #173082 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

        Attachment #173082 -
        Flags: superreview?(bzbarsky)

        Attachment #173082 -
        Flags: superreview-

        Attachment #173082 -
        Flags: review?(bzbarsky)

        Attachment #173082 -
        Flags: review-

    
    

    
  
Boris, i haven't yet looked at his patch... In what way it break selections on
mac; wrong color or something else?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv5.0 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #173082 -
        Attachment is obsolete: true

        Attachment #173195 -
        Flags: superreview?(bzbarsky)

        Attachment #173195 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

        Attachment #173195 -
        Flags: superreview?(bzbarsky)

        Attachment #173195 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv6.0 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #173195 -
        Attachment is obsolete: true

        Attachment #173248 -
        Flags: superreview?(bzbarsky)

        Attachment #173248 -
        Flags: review?(bzbarsky)

    
    

    
  
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 ;-)

        Attachment #173248 -
        Flags: superreview?(bzbarsky)

        Attachment #173248 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv6.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #173248 -
        Attachment is obsolete: true

        Attachment #173265 -
        Flags: superreview?(bzbarsky)

        Attachment #173265 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

        Attachment #173265 -
        Flags: superreview?(bzbarsky)

        Attachment #173265 -
        Flags: superreview-

        Attachment #173265 -
        Flags: review?(bzbarsky)

        Attachment #173265 -
        Flags: review-

    
    

    
  
> 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.

    
    

    
  
> 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.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv6.2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #173265 -
        Attachment is obsolete: true

        Attachment #173354 -
        Flags: superreview?(bzbarsky)

        Attachment #173354 -
        Flags: review?(bzbarsky)

    
    

    
  
> 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....

    
    

    
  
I believe long ago that we decided that Mac users didn't like it if you messed
with their selection color in any way.
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha

    
    

    
  
(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.

    
    

    
  
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.)
Target Milestone: mozilla1.9alpha → mozilla1.8beta1
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha

        Attachment #173354 -
        Flags: superreview?(bzbarsky)

        Attachment #173354 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv7.0
        — 
          — Splinter Review
      

    


  

        Attachment #174370 -
        Flags: superreview?(bzbarsky)

        Attachment #174370 -
        Flags: review?(bzbarsky)

        Attachment #173354 -
        Attachment is obsolete: true

    
    

    
  
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 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?

        Attachment #174370 -
        Flags: review?(bzbarsky) → review?(sfraser_bugs)

        Attachment #174370 -
        Flags: review?(sfraser_bugs) → review+

    
    

    
  
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 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.... :(

        Attachment #174370 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
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.

    
    

    
  
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9alpha → mozilla1.8beta2

    
    

    
  
Actually, if you have a better idea file a _new_ bug.

    
    

    
  
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+

    
    

    
  
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.

    
    

    
  
See bug 289181...

    
    

    
  
Er.. Charles, what does that cache service bug have to do with this bug?

    
    

    
  
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.

    
    

    
  
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.
Depends on: 289191

    
    

    
  
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.

    
    

    
  
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



    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
We could also just find the closest non-transparent background in general... I
believe we have utility functions for this, no?

    
    

    
  


  

        Attachment #179838 -
        Flags: superreview?(bzbarsky)

        Attachment #179838 -
        Flags: review?(bzbarsky)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.8beta2 → ---
Status: REOPENED → ASSIGNED

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  

        Attachment #164817 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        testcase2
        — 
      

    


  

        Attachment #83458 -
        Attachment is obsolete: true

        Attachment #172879 -
        Attachment is obsolete: true

    
    

    
  
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.

        Attachment #179838 -
        Flags: superreview?(bzbarsky)

        Attachment #179838 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv8.0 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #179838 -
        Attachment is obsolete: true

        Attachment #179869 -
        Flags: superreview?(bzbarsky)

        Attachment #179869 -
        Flags: review?(bzbarsky)

    
    

    
  
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/

        Attachment #179869 -
        Flags: superreview?(bzbarsky)

        Attachment #179869 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv8.1 (obsolete)
        — 
          — Splinter Review
      

    


  
Oops..
I have a mistake.

        Attachment #179869 -
        Attachment is obsolete: true

        Attachment #179872 -
        Flags: superreview?(bzbarsky)

        Attachment #179872 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

    
    

    
  
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.
Flags: blocking1.8b2?
Priority: P3 → P1
Target Milestone: --- → mozilla1.8beta2

    
    

    
  
(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 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)

        Attachment #179838 -
        Attachment is obsolete: false

        Attachment #179838 -
        Flags: superreview?(bzbarsky)

        Attachment #179838 -
        Flags: review?(bzbarsky)

    
    

    
  
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

    
    

    
  
>>+  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.

    
    

    
  
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.

    
    

    
  
Oops..

You're right.
I missed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv8.2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #179872 -
        Attachment is obsolete: true

        Attachment #179964 -
        Flags: superreview?(bzbarsky)

        Attachment #179964 -
        Flags: review?(bzbarsky)

        Attachment #179872 -
        Flags: superreview?(bzbarsky)

        Attachment #179872 -
        Flags: review?(bzbarsky)

    
    

    
  
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...

        Attachment #179964 -
        Flags: superreview?(dbaron)

        Attachment #179964 -
        Flags: superreview?(bzbarsky)

        Attachment #179964 -
        Flags: review?(bzbarsky)

        Attachment #179964 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for check-in (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #179966 -
        Attachment is obsolete: true

        Attachment #179838 -
        Flags: superreview?(bzbarsky)

        Attachment #179838 -
        Flags: review?(bzbarsky)

    
    

    
  
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.


    
    

    
  
(In reply to comment #100)

I think that the idea of previous comment is nice idea.
Thanks. Please wait. I'm testing new patch.

        Attachment #179964 -
        Flags: superreview?(dbaron)

        Attachment #179964 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rv9.0 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch looks good for me better than rv8.2.

        Attachment #179964 -
        Attachment is obsolete: true

        Attachment #179967 -
        Attachment is obsolete: true

        Attachment #180069 -
        Flags: superreview?(dbaron)

        Attachment #180069 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

        Attachment #180069 -
        Flags: superreview?(dbaron)

        Attachment #180069 -
        Flags: review?(bzbarsky)

        Attachment #180069 -
        Attachment is obsolete: true

        Attachment #180079 -
        Flags: superreview?(dbaron)

        Attachment #180079 -
        Flags: review?(bzbarsky)
Blocks: 289191, 289287
No longer depends on: 289191

    
    

    
  
If the patch is checked-in, the fixed bug list is this, bug 176605, bug 289191
and bug 289287.

    
    

    
  
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);


    
    

    
  
(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.



    
    

    
  
Sure.  But the point is, people are using such colors for their selections,
because they _do_ use color devices...

    
    

    
  
(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.

    
    

    
  


  
I cannot watch this screenshot while long time....
Depends on: 289652

    
    

    
  
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.

    
    

    
  
Can we detect what sort of device we're running on, perhaps?  And expose that on
the prescontext or something?

    
    

    
  
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.

        Attachment #180300 -
        Attachment is obsolete: true

    
    

    
  
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...

    
    

    
  
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.

    
    

    
  
Ivan, please do take a look at the patch before making code suggestions....

    
    

    
  
(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 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...

        Attachment #180079 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Thank you, Boris.
I will create new patch in _new bug_ after this bug fixed...

    
    

    
  
I have been sent mail to dbaron. But he doesn't superreview yet...
Boris, can you superreview it?

    
    

    
  
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.

        Attachment #180079 -
        Flags: superreview?(dbaron)

        Attachment #180079 -
        Flags: superreview+

        Attachment #180079 -
        Flags: approval1.8b2?

        Attachment #180079 -
        Flags: approval1.8b2? → approval1.8b2+

    
    

    
  
Checked-in to Trunk.
Thanks.


*Note that don't reopen this bug.*
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 19 years ago
Flags: blocking1.8b2?
Resolution: --- → FIXED

    
    

    
  
I opened bug 290919 that is advanced algorithm for color device.

    
    

    
  
Related: bug 111526 (outline color)

    
    

    
  
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.

    
    

    
  
(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



    
    

    
  
What build are you running?  Is it trunk?  What OS?

    
    

    
  
Hmm, works fine on home system, which is winxp.  Will double check build and try
new profile on my win2k system tomorrow.

    
    

    
  
> 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... :(

    
    

    
  
(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+


    
    

    
  
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.

    
    

    
  
> 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?

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
s/unreadable/undistinguishable/

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot
        — 
      

    


  
rbs:
I cannot reproduce it.
My system is Win2k(default selection colors).

        Attachment #181395 -
        Attachment is patch: false

        Attachment #181395 -
        Attachment mime type: text/plain → image/png

        Attachment #181395 -
        Attachment mime type: image/png → image/jpeg

    
    

    
  
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
}

    
    

    
  
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].

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.
Whiteboard: Don't add comment for new problem. See #146.
Depends on: 332612
No longer blocks: 292191
No longer depends on: 345210
No longer depends on: 332612

    
    

    
      
  
Flags: needinfo?(swill8298)
Restrict Comments: true

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: