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

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
Selection
P1
normal
RESOLVED FIXED
17 years ago
3 years ago

People

(Reporter: Scott Gifford (old account), Assigned: masayuki)

Tracking

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

Trunk
mozilla1.8beta2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(10 attachments, 19 obsolete attachments)

17.17 KB, patch
Simon Fraser
: review+
Details | Diff | Splinter Review
17.07 KB, patch
Details | Diff | Splinter Review
1.80 KB, patch
Details | Diff | Splinter Review
257.11 KB, text/html
Details
276.60 KB, text/html
Details
7.51 KB, patch
Details | Diff | Splinter Review
1.54 KB, image/png
Details
285.77 KB, image/jpeg
Details
31.51 KB, image/png
Details
87.28 KB, image/jpeg
Details
(Reporter)

Description

17 years ago
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

17 years ago
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

Comment 2

17 years ago
hmm..I don't know of one. Looking...

Comment 3

17 years ago
Not going to get to this by RTM, future.
Target Milestone: --- → Future

Comment 4

15 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 5

15 years ago
*** Bug 91708 has been marked as a duplicate of this bug. ***

Comment 6

15 years ago
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

Comment 7

15 years ago
Created attachment 83458 [details]
testcase

Comment 8

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

Comment 9

15 years ago
*** Bug 162478 has been marked as a duplicate of this bug. ***
*** Bug 164703 has been marked as a duplicate of this bug. ***

Comment 11

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

Comment 12

15 years ago
4xp as per comment 6
Keywords: 4xp

Updated

15 years ago
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. ***

Updated

14 years ago
Blocks: 140580

Comment 15

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

Comment 16

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

Comment 17

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

Updated

13 years ago
Blocks: 164421
*** Bug 254844 has been marked as a duplicate of this bug. ***

Comment 19

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

Updated

13 years ago
Blocks: 255941
Created attachment 164736 [details] [diff] [review]
Patch

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

Updated

13 years ago
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.
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.
Attachment #164736 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #164816 - Flags: superreview?(roc)
Attachment #164816 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #164736 - Flags: superreview?(roc)
Attachment #164736 - Flags: review?(bzbarsky)
Created attachment 164817 [details]
testcase
testbuild.
http://www.d-toybox.com/mozilla/testbuilds/bug2909.zip
Created attachment 164822 [details] [diff] [review]
Patch rv2.1
Attachment #164816 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #164816 - Flags: superreview?(roc)
Attachment #164816 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
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

Comment 34

13 years ago
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.
(Assignee)

Updated

13 years ago
Blocks: 113161
(Assignee)

Updated

13 years ago
Depends on: 234102
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
Created attachment 172879 [details]
another testcase
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;
}
Created attachment 173076 [details] [diff] [review]
Patch rv3.0(work in progress)
(Assignee)

Updated

12 years ago
Attachment #173076 - Attachment description: Patch rv1.3(work in progress) → Patch rv3.0(work in progress)
Created attachment 173082 [details] [diff] [review]
Patch rv4.0

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?
See comment 41.
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.
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.
(Assignee)

Updated

12 years ago
Attachment #173195 - Flags: superreview?(bzbarsky)
Attachment #173195 - Flags: review?(bzbarsky)
Created attachment 173248 [details] [diff] [review]
Patch rv6.0
Attachment #173195 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
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 ;-)
(Assignee)

Updated

12 years ago
Attachment #173248 - Flags: superreview?(bzbarsky)
Attachment #173248 - Flags: review?(bzbarsky)
Created attachment 173265 [details] [diff] [review]
Patch rv6.1
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.
Created attachment 173354 [details] [diff] [review]
Patch rv6.2
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....

Comment 57

12 years ago
I believe long ago that we decided that Mac users didn't like it if you messed
with their selection color in any way.
(Assignee)

Updated

12 years ago
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
(Assignee)

Updated

12 years ago
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha
(Assignee)

Updated

12 years ago
Attachment #173354 - Flags: superreview?(bzbarsky)
Attachment #173354 - Flags: review?(bzbarsky)
Created attachment 174370 [details] [diff] [review]
Patch rv7.0
Attachment #174370 - Flags: superreview?(bzbarsky)
Attachment #174370 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
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)

Updated

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

Comment 63

12 years ago
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+
Created attachment 179497 [details] [diff] [review]
Patch for check-in
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Target Milestone: mozilla1.9alpha → mozilla1.8beta2
Actually, if you have a better idea file a _new_ bug.

Comment 69

12 years ago
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.

Comment 71

12 years ago
See bug 289181...
Er.. Charles, what does that cache service bug have to do with this bug?

Comment 73

12 years ago
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?
Created attachment 179838 [details] [diff] [review]
Patch for disable previous patch
Attachment #179838 - Flags: superreview?(bzbarsky)
Attachment #179838 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.8beta2 → ---
(Assignee)

Updated

12 years ago
Status: REOPENED → ASSIGNED
Created attachment 179846 [details]
testcase
Attachment #164817 - Attachment is obsolete: true
Created attachment 179849 [details]
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)
Created attachment 179869 [details] [diff] [review]
Patch rv8.0
Attachment #179838 - Attachment is obsolete: true
Attachment #179869 - Flags: superreview?(bzbarsky)
Attachment #179869 - Flags: review?(bzbarsky)
This patch fixes bug 289191 and bug 176605.
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/
(Assignee)

Updated

12 years ago
Attachment #179869 - Flags: superreview?(bzbarsky)
Attachment #179869 - Flags: review?(bzbarsky)
Created attachment 179872 [details] [diff] [review]
Patch rv8.1

Oops..
I have a mistake.
Attachment #179869 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #179872 - Flags: superreview?(bzbarsky)
Attachment #179872 - Flags: review?(bzbarsky)

Comment 88

12 years ago
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.
Created attachment 179964 [details] [diff] [review]
Patch rv8.2
Attachment #179872 - Attachment is obsolete: true
Attachment #179964 - Flags: superreview?(bzbarsky)
Attachment #179964 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
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+
Created attachment 179966 [details] [diff] [review]
Patch for check-in
Created attachment 179967 [details] [diff] [review]
Patch for check-in
Attachment #179966 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #179838 - Flags: superreview?(bzbarsky)
Attachment #179838 - Flags: review?(bzbarsky)

Comment 100

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #179964 - Flags: superreview?(dbaron)
Attachment #179964 - Flags: review+
Created attachment 180069 [details] [diff] [review]
Patch rv9.0

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)
Created attachment 180079 [details] [diff] [review]
Patch rv10.0
(Assignee)

Updated

12 years ago
Attachment #180069 - Attachment is obsolete: true
Attachment #180079 - Flags: superreview?(dbaron)
Attachment #180079 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
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...
Created attachment 180148 [details]
On colorful device, monochrome device and if the user color-blindness(2 patterns)
(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.
Created attachment 180150 [details]
screen shot of comment 108

I cannot watch this screenshot while long time....

Updated

12 years ago
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.
Created attachment 180300 [details] [diff] [review]
Patch rv11.0(test patch, I don't like this)
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...

Comment 117

12 years ago
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
Last Resolved: 12 years ago12 years ago
Flags: blocking1.8b2?
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 290919
I opened bug 290919 that is advanced algorithm for color device.

Comment 126

12 years ago
Related: bug 111526 (outline color)

Comment 127

12 years ago
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

12 years ago
(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

12 years ago
What build are you running?  Is it trunk?  What OS?

Comment 130

12 years ago
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... :(

Comment 132

12 years ago
(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

12 years ago
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?

Comment 135

12 years ago
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

12 years ago
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

12 years ago
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.

Comment 139

12 years ago
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

12 years ago
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?
Created attachment 181395 [details]
screenshot

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

Updated

12 years ago
Attachment #181395 - Attachment is patch: false
Attachment #181395 - Attachment mime type: text/plain → image/png
(Assignee)

Updated

12 years ago
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
}

Comment 143

12 years ago
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].
(Assignee)

Updated

12 years ago
Blocks: 292191

Comment 144

12 years ago
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

12 years ago
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: 322798

Updated

11 years ago
Depends on: 332612
Depends on: 345210
(Assignee)

Updated

11 years ago
No longer blocks: 292191
No longer depends on: 345210
(Assignee)

Updated

11 years ago
Depends on: 292191
(Assignee)

Updated

11 years ago
No longer depends on: 332612
Duplicate of this bug: 208693
You need to log in before you can comment on or make changes to this bug.