Closed Bug 56314 Opened 24 years ago Closed 20 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
(deleted), patch
sfraser_bugs
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch for check-in
(deleted), patch
Details | Diff | Splinter Review
Patch for disable previous patch
(deleted), patch
Details | Diff | Splinter Review
testcase
(deleted), text/html
Details
testcase2
(deleted), text/html
Details
Patch rv10.0
(deleted), 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)
(deleted), image/png
Details
screen shot of comment 108
(deleted), image/jpeg
Details
screenshot: comparison of the selection before&after the patch
(deleted), image/png
Details
screenshot
(deleted), 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) (deleted) —
*** 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) (deleted) — — 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) (deleted) — — 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) (deleted) —
Attached patch Patch rv2.1 (obsolete) (deleted) — — 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) (deleted) —
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; }
Attached patch Patch rv3.0(work in progress) (obsolete) (deleted) — — Splinter Review
Attachment #173076 - Attachment description: Patch rv1.3(work in progress) → Patch rv3.0(work in progress)
Attached patch Patch rv4.0 (obsolete) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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 (deleted) — — 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: 20 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 (deleted) —
Attachment #164817 - Attachment is obsolete: true
Attached file testcase2 (deleted) —
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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — Splinter Review
Attached patch Patch for check-in (obsolete) (deleted) — — 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) (deleted) — — 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.
Attached image screen shot of comment 108 (deleted) —
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: 20 years ago → 20 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 (deleted) —
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: