User-Agent: Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040120 Firebird/0.8.0+ This css code p::-moz-selection{} would make any selection of p elements a transparent color. Also the font color stays black, as opposed to 'normal' circumstances where the font color becomes white. Most likely the css properties background-color and color (and others) get the value inherit for the ::-moz-selection. Reproducible: Always Steps to Reproduce: 1.See testcase and select some text 2. 3. Actual Results: The p tag gets no selection background color and the text stays black Expected Results: I would expect the default behavior of Mozilla. The background color of the selection should become darkish blue, and the font color should become white. Possibly you could call these default properties, which can only be changed when they were specifically stated with the corresponding css properties, not with an empty ::-moz-selection{}

The issue here is that DrawSelectionIterator::DrawSelectionIterator just looks for styles for a ::selection. If it finds them, it uses them. In this case, it finds them (the color is inherited from the parent, the background is "none", etc). So the real problem is that the normal selection behavior is NOT described in CSS, so we have this weird discontinuity where just the presence of a ::selection selector suddenly means that we find style for the selection iterator to use. It seems to me that moving our normal selection styling to using a "*::selection" selector in ua.css is the way to go here. There's the issue of active vs inactive selections, but we may be able to handle that with a -moz "system color" or something....

Note that ::selection should inherit styles from the selected element rather than his parent. Note entirely sure if this is relevant for this specific bug, but it is relevant for ::selection support. See also: <http://www.hixie.ch/tests/adhoc/css/selectors/selection/mozilla/003.xml> (If this is as easy as having ::-moz-selection in ua.css, please confirm, I can create a patch for this bug.)

Anne, you can try doing that and seeing what breaks... I suspect something will, but we won't know what till someone tries.

This will fix this bug, though still leaves much to be desired in terms of ::selection support (Ian's test still fails)

Comment on attachment 160121 [details] [diff] [review] add to ua.css No, this is wrong. See my comments about active vs inactive selections. Also, note that a patch to do this should result in the removal of the C++ code that's doing the job now.

re-looking at the code, we have no way (that I can see) in Cpp to (currently) distinguish a disabled selection state using CSS... DrawSelectionIterator::DrawSelectionIterator (http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#984) ---only stores mSelectionPseudoBGcolor (in regards to background color, besides the look-and feel stuff) DrawSelectionIterator::CurrentBackGroundColor (http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#1219) ---uses the bits in nsISelectionController to choose between the "active" selection color, or the "Disabled" one from the look & feel. (does output the PsuedoStyled bgcolor as listed in the init) If this is all irrelevant and I am forgetting a selector that will work for that case, please tell me here and I can take care of it. As a second though bz, do we want to Remove all this look-and-feel color getting from the code? Also, what should we use as a "fallback" incase we dont find a ::selection color?

...might as well accept too....

(In reply to comment #7) > re-looking at the code, we have no way (that I can see) in Cpp to (currently) > distinguish a disabled selection state using CSS... See comment 2 the part about 'we may be able to handle that with a -moz "system color" or something'. That is a -moz-standard-selection color that will get painted right by the back end as needed. (http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#1219) > ---uses the bits in nsISelectionController to choose between the "active" > selection color, or the "Disabled" one from the look & feel. Right. And if the color is -moz-standard-selection that's what we should do, hopefully. There's the issue of knowing that from the computed color (which is an nscolor, last I checked). > As a second though bz, do we want to Remove all this look-and-feel color > getting from the code? Doesn't sound like it, does it? > Also, what should we use as a "fallback" incase we dont find a > ::selection color? Don't paint the selection? Or do what we do now?

Comment on attachment 160121 [details] [diff] [review] add to ua.css bz, I just tested this again on my Firefox build really quick after staring at the TextFrame code for getting the background color. And as I looked and expected, it seems that we wont ruin the disabled selection state with this patch at all, it is generated and chosen with a higher priority than ::-moz-selection, than NORMAL selection, and then our FAYT back-color is the only thing which "beats" it in that precidence war. I would personally opt for keeping current functionality if ::-moz-selection is missing, though I can remove current Cpp for it's current color computation if you like.

Comment on attachment 160121 [details] [diff] [review] add to ua.css The point is that you're introducing two completely separate codepaths that do exactly the same thing. We'll go to all the trouble of computing selection style on everything, but we still have code lying about (unused now) that figures out the "normal" selection color... Re-looking at the code, I agree that the way the C++ is hacked the active/disabled colors will override the ::-moz-selection color altogether. It's not clear to me whether that's desirable, but let's stick with it. So this ua.css change would be OK if it were accompanied by corresponding back-end changes to remove the now-unneeded code. Also, setting "background: transparent" on ::-moz-selection will kill off active/disabled backgrounds. Is that desired?

This removes redundant code, though unfortunately I could not remove the Look&Feel call for the foreground color, (almost did), though realized that on Mac machines at least, foreground color changing on selection is considered 'bad' (if not broken).

I'm not going to get to this anytime soon (not within a week at least).

as a side-note Ian's adhoc test now does "Pass" in Mozilla with the exeception of cursor inside a ::-moz-selection. [did not test outline here, I do not currently build with outline support]

Comment on attachment 162437 [details] [diff] [review] ua.css + Removal of Cpp for same Note: please use -pu8 or something for your diff (-p and more context). >Index: layout/html/document/src/ua.css >+ background: Highlight; >+ color: HighlightText; Note that these are not the same thing as textSelectBackground and textSelectForeground look-and-feel values that you're replacing. If nothing else, they are controlled by different prefs... Please file a followup bug to eliminate uses of textSelectBackground/textSelectForeground altogether? There are various users in the tree that should be looking at ::selection (eg nsMathMLChar.cpp, nsFrame.cpp, nsSVGGlyphFrame.cpp, nsTableCellFrame.cpp). >Index: layout/html/base/src/nsTextFrame.cpp >@@ -588,8 +588,7 @@ >+ PRBool mSelColorCanChange; What does this member mean? Document, please. >+ // Determine if Foreground Color can change Don't capitalize "foreground color". >+ if (foreColor == NS_DONT_CHANGE_COLOR) >+ mSelColorCanChange = PR_FALSE; >+ else >+ mSelColorCanChange = PR_TRUE; How about: mSelColorCanChange = (foreColor != NS_DONT_CHANGE_COLOR); >@@ -1001,6 +1002,13 @@ >+ // Get background colors for disabled selection at attention-getting selection (used with type ahead find) The "at" should be an "and" I think... I know you just moved this, but fix it, please. >+ mDisabledColor = EnsureDifferentColors(mDisabledColor, >+ mSelectionPseudoBGcolor); >+ mAttentionColor = EnsureDifferentColors(mAttentionColor, >+ mSelectionPseudoBGcolor); Why the change to what color we're ensuring we're different from? What about the case when the pseudo bg _is_ transparent? We used to EnsureDifferentColor unconditionally; why is that not needed? Document this in the patch, maybe? That is, explain _why_ all this is being done this way in code comments. >@@ -1189,28 +1192,23 @@ > DrawSelectionIterator::CurrentForeGroundColor() >+ if (useSelColor && (mOldStyle.mSelColorCanChange)) { No need for extra parens about second boolean. >- else >- return foreColor; Why? This used to return mOldStyle.mSelectionTextColor, but now we'll return mOldStyle.mColor->mColor in the same case... is this correct? Or is mSelectionPseudoStyle always non-null now with the ua.css change? If so, why do we need a null-check here? >@@ -1219,17 +1217,20 @@ > DrawSelectionIterator::CurrentBackGroundColor(nscolor &aColor, PRBool *aIsTransparent) Similar here... if mSelectionPseudoStyle is never null, why the checks, and otherwise why the lack of handling of that case? >- *aIsTransparent = PR_FALSE; >+ *aIsTransparent = PR_TRUE; Why this change? r-, but the next review should be much faster.... I'm sorry this one took as long as it did.

>Note: please use -pu8 or something for your diff (-p and more context). Not a problem will do >>Index: layout/html/document/src/ua.css > >>+ background: Highlight; >>+ color: HighlightText; > >Note that these are not the same thing as textSelectBackground and >textSelectForeground look-and-feel values that you're replacing. If nothing >else, they are controlled by different prefs... Would you prefer these value's or a new CSS color keyword for textSelect[Foreground|Background]? >Please file a followup bug to eliminate uses of >textSelectBackground/textSelectForeground altogether? There are various users >in the tree that should be looking at ::selection (eg nsMathMLChar.cpp, >nsFrame.cpp, nsSVGGlyphFrame.cpp, nsTableCellFrame.cpp). Will do after this is done ;-) >Index: layout/html/base/src/nsTextFrame.cpp >>+ mDisabledColor = EnsureDifferentColors(mDisabledColor, >>+ mSelectionPseudoBGcolor); >>+ mAttentionColor = EnsureDifferentColors(mAttentionColor, >>+ mSelectionPseudoBGcolor); > >Why the change to what color we're ensuring we're different from? What about >the case when the pseudo bg _is_ transparent? We used to EnsureDifferentColor >unconditionally; why is that not needed? Before we were Ensuring different only from the "default" background color, this way we are checking if we are different from the actual ::[-moz-]selection background color, though only if we are not transparant. After my patch without either a page rule, or a ua.css rule there will be no background color applied for selections. >Document this in the patch, maybe? That is, explain _why_ all this is being >done this way in code comments. Sure thing, if I can come up with the best wording (note, when my next patch is to be checked in, a comment change here would be rather trivial, and I would personally ok a clarification without approval, just so long as someone attaches a new patch as "Actually Checked In" or some such. >>- else >>- return foreColor; > >Why? This used to return mOldStyle.mSelectionTextColor, but now we'll return >mOldStyle.mColor->mColor in the same case... is this correct? > >Or is mSelectionPseudoStyle always non-null now with the ua.css change? If so, >why do we need a null-check here? With this patch the only chance to style a selection is via a .css file, with this patch we assume, but cannot be 100% sure that ua.css has a style, as such we do not compute a foreground color in the code directly. We do check it briefly to ensure that the look and feel code does not prohibit us from changing the foreground color, (such as on a Mac). If there is no ::[-moz-]selection set we will fall back to the normal color, as determined if it was not selected... and with ua.css this would _never_ change most users experience, unless the choose to modify ua.css. >>@@ -1219,17 +1217,20 @@ >> DrawSelectionIterator::CurrentBackGroundColor(nscolor &aColor, PRBool >*aIsTransparent) > >Similar here... if mSelectionPseudoStyle is never null, why the checks, and >otherwise why the lack of handling of that case? See above >>- *aIsTransparent = PR_FALSE; >>+ *aIsTransparent = PR_TRUE; > >Why this change? I'll have to re-look at callers to remember why I made this change, I do recall studying stuff to make this choice though. Its been a bit too long to remember emmediately...though I will explain in my next patch (or just plain revert it, if I dont know why). all other review comments are noted, and will be addressed.

(In reply to comment #16) > Would you prefer these value's or a new CSS color keyword for > textSelect[Foreground|Background]? These values are OK; like I said, I think we should try to get rid of the textSelect* values. > Before we were Ensuring different only from the "default" background color, this > way we are checking if we are different from the actual ::[-moz-]selection > background color, though only if we are not transparant. Given that the ::-moz-selection background now _is_ the selection background, perhaps the member should be renamed (by dropping "pseudo")? > With this patch the only chance to style a selection is via a .css file, with > this patch we assume, but cannot be 100% sure that ua.css has a style, So... we will always have a ua.css. But we may not get a pseudo context due to OOM or something like that, of course. I would say it would make more sense to simply not go through selection painting at all if we fail to get a ::-moz-selection style context, instead of executing all that code but not painting anything useful. That would make the flow of control much clearer and allow removal of these random null-checks.

(In reply to comment #17) >I would say it would make more sense to simply not go through selection > painting at all if we fail to get a ::-moz-selection style context [snip] Ok, but this would require as well a ::-moz-disabled-selection and a ::-moz-attention-selection, (or some such) would it not? currently those are hardcoded in...as I remember it. I dont think that is relevant for this bug, but I may be wrong. Currently we cope (with my patch) by generating the ::-moz-selection color things on the textFrame creation, and when painting querying for the color, if selected we use the selected color. unless I misunderstand the code-paths in my tired state (and without staring at the code)

> Ok, but this would require as well a ::-moz-disabled-selection and a > ::-moz-attention-selection, (or some such) would it not? No. Right now (with your patch), the only way to not have a ::-moz-selection style context is if we ran out of memory or someone messed with ua.css. In either case, we should just disable painting of selections entirely.

bz, with my code the background color can _still_ change regardless of if there is something in ua.css, the foreground color does not... https://bugzilla.mozilla.org/attachment.cgi?id=162437&action=diff#mozilla/layout/html/base/src/nsTextFrame.cpp_sec6 If I am still wrong, can you please (if you have time) explain to me why. on another note, the >+ *aIsTransparent = PR_TRUE; is so that if we are a selection, but there is no value in ua.css, and we are not a disabled selection, or an "attention" (FAYT) selection, which we have hard-code for, that we tell the caller that we are transparant for the background. Which is safe to assume... due to background-color: transparant being the default for background-color in normal CSS. IMO. Called from (rv 1.490): http://lxr.mozilla.org/mozilla/source/layout/generic/nsTextFrame.cpp#2443 http://lxr.mozilla.org/mozilla/source/layout/generic/nsTextFrame.cpp#3089 http://lxr.mozilla.org/mozilla/source/layout/generic/nsTextFrame.cpp#3310

> regardless of if there is something in ua.css The point is, there _will_ be something in ua.css. We put it there. Editing of that file is not supported; most changes to it will lead to totally incorrect behavior, sometimes including crashes. So the only way we will be missing the style context for the selection is if we run out of memory. In which case, it makes more sense to disable selection painting entirely than to paint some types of selections but not others.... For the aTransparent thing, comment it in your patch (but note that we should not be calling that code if there is no selection pseudo, per comment above).

>The point is, there _will_ be something in ua.css. We put it there. Editing of that file is not >supported; most changes to it will lead to totally incorrect behavior, sometimes including >crashes. I guess I can live with that mantra, though I still wonder why its really needed when realistically ::-moz-selection { ... } does not determine if there will or will not be a background-color drawn (exclusively), [due to the attention and disabled stuff as stated above] Though I would ask that you allow this patch as-will-be and have the "not call this function..." stuff for another bug.

No, that needs to be addressed before I'd be willing to OK these changes... See comment 6 paragraph 2. That still applies.

/me really wishes you were on IRC right now... as far as I understand it, I am removing the C++ code that is doing the job, adding a check for it in the painting routine would only (as I see it) add extra computation, as each time it tries to paint for a TextFrame it will check if there is a Pseudo-Style applied, where the other way it will have already peeked into the style-tree and checked, and stored it as the member variable, doing less work as I see it. Especially since ua.css is now actually setting ::-moz-selection all the time, so if we are selected we technically _should_ be calling this function, and since we already do a check to verify that there is a Pseudo (where I would personally expect the check) why the need to add the check around the call to this function only to protect against ua.css edits? as "Editing of that file is not supported; most changes to it will lead to totally incorrect behavior, sometimes including crashes."~c#21 I guess my point is that I still dont really see the need for an additional check on if the Pseudo is present, since as this code is now, it works either way and this function _should_ be called with the ua.css we have (after my patch)

(In reply to comment #24) > as far as I understand it, I am removing the C++ code that is doing the job You are. I'm just not happy with the quality of the removal. > adding a check for it in the painting routine would only (as I see it) add extra > computation, as each time it tries to paint for a TextFrame it will check if > there is a Pseudo-Style applied, where the other way it will have already peeked > into the style-tree and checked, and stored it as the member variable, doing > less work as I see it. Right now you do a null-check for mPseudo* stuff in several places in the DrawSelectionIterator... I suggest you do it only once, somewhere (not sure where, offhand) and bypass most of the selection iterator code altogether if we don't have a style context there. I can try to pinpoint the exact place where I would put the check, but that's not happening till I have a normal net connection (Jan 3). > since we already do a check to verify that there is a Pseudo We do? What happens if there isn't one? > only to protect against ua.css edits? We're protecting against out-of-memory conditions, not ua.css edits. > I guess my point is that I still dont really see the need for an additional > check on if the Pseudo is present, since as this code is now, it works either > way and this function _should_ be called with the ua.css we have (after my patch) Right now there are multiple null checks scattered through this code. I am suggesting we remove them all and have _one_ null check that we do at one spot, before even bothering to enter this code, and skip the code if the null-check fails. I'm not suggesting "additional" null-checks, but rather fewer null-checks.

With the changes checked in by bug 56314 we no longer want something in ua.css I assume? Or do we have moz-values that are equivalent to the selection behavior being landed there?

We would create such -moz-* values, presumably. Per discussion in this bug, we probably needed those anyway.

There's also some pointers related to this issue in bug 509958 comment 2.

It's been quite a while, and the codebase has changed a fair bit, but I've updated the patch here so that it adds -moz-prefixed colors related to selection, and uses them in the UA stylesheet. I'm not sure if that's the approach we'd like to take these days, so let me know if there's something else we'd like to do here. Note that I don't really know if there is any code that should still be removed from the related classes, as the code is quite different from before. Also note that one existing reftest needed tweaking due to this change (the one at the end of the patch), as its colors no longer match the reference image. However, they do match Chrome's output, and Chrome does not prefix ::selection, so I'm guessing it might not be a problem. If it is then we'll need to consider this a bit more. Since I'm not 100% sure whether this is the approach we'd like to go with, I'm not requesting a full review yet, but do let me know if it's still on the right track.

I don't see how that ever ends up painting the inactive background color for the selection... Apart from that, I'm really not sure how this stuff works nowadays either. I haven't looked at it in a long time. Mats might know more about it.

Comment on attachment 8843783 [details] [diff] [review] 234102-add_default_moz_selection_which_uses_moz_specific_colors.diff > +#define NS_COLOR_MOZ_SELECTION_FOREGROUND -19 I think you meant -9 here. Please align all these values so that they line up with -8 in this patch. (It makes it a little easier to spot typos like that.) Also, I'm not sure I feel about exposing yet another non-standard -moz-prefixed color value to the web, but I suspect from a fingerprinting POV it's no worse than the colors we already expose. Perhaps we should just try to standardize these names? ;-) I tested this patch in a local Linux build and it seems to work fine for me, except for one regression: when selecting text that is already the same or similar color/bgcolor to the selection colors, then the old behavior is that we flip the selection colors. With this patch we don't. So I suspect we have some C++ code in nsTextFrame.cpp that tests for that case, but that it only tests it if no ::-moz-selection style was found. I think we need to do something about that, not sure what though. (I suspect that is the reason why non-themed-widget.html "broke".) I'll attach a testcase...

Select some text on each of these lines and compare the old/new behavior. Compare also the inactive selection colors by focusing the URL bar.

BTW, does anyone know if other UAs have prefixed names for selection colors?

(In reply to Thomas Wisniewski from comment #32) > Also note that one existing reftest needed tweaking due to this change (the > one at the end of the patch), as its colors no longer match the reference > image. However, they do match Chrome's output [...] Fwiw, it doesn't match Chrome on Linux for me as this screenshot shows.

The code that sets up the selection colors lives here: http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/generic/nsTextFrame.cpp#4021 I suspect the regression I described in comment 34 comes from |sc| always being non-null now since we have the ::-moz-selection in the UA sheet matches all elements. I think we can workaround that by testing if the foreground/background colors are exactly those in the UA sheet and treat that as |sc == nullptr|, or add some EnsureDifferentColors calls there. Thomas, is this something you could take on? It'd be nice to finish this off now that we're so close! :-)

Yes, I'm definitely willing (in fact I'd like to try to get moz-selection unprefixed ASAP). The end of the quarter is just looming overhead right now, so I may not get to it for a week or two. Thanks for the analysis, and if you or anyone else is itching to get it done before I can, just say so :)

> I think we can workaround that by testing if > the foreground/background colors are exactly those in the UA sheet That seems a bit fragile; if a page gets those via getComputedStyle and then sets them itself, it will have different behavior from what we used to have... In the Gecko world, we can probably do this via checking whether all rulenodes for the style context are UA rules or something. I'm not sure whether we have some equivalent for stylo, though.

Actually, I found a few minutes tonight and this was nagging at me, so I took a quick look. It turns out that the other code-path in that function mats pointed out basically just calls EnsureSufficientContrast() to swap the colors if the bgcolors are too similar. I've confirmed that just calling it as well in the "if (sc)" clause does the trick for mats' test case, too (in comment 35). I'm not sure if that's enough to allay bz's concern, but it seems to me that it may. I'll roll a fresh patch integrating the test-case in comment 35 and give it a try-run ASAP.

Comment on attachment 8843783 [details] [diff] [review] 234102-add_default_moz_selection_which_uses_moz_specific_colors.diff Review of attachment 8843783 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/res/ua.css @@ +10,5 @@ > */ > > +%ifdef XP_MACOSX > +*|*::-moz-selection { > + background-color:-moz-selection-background-color-active; Why only background color is changed on macOS? Doesn't it make text unreadable in some edge cases?

We can get the RuleNode for an Element, and get the CascadeLevel from the RuleNode.

We don't have an Element. We have a ::selection pseudo-element.

emk, that's just how selections work on OSX; the foreground color isn't changed. This is true in other browsers as well. Yes, it does make text unreadable in some cases on other browsers, but we change the foreground color if it's too close to the selection color to ensure contrast (my final patch will ensure this is still happening). bz, this is how the code currently gets the background color of the frame, so it can compare it with the selection background color: nsIFrame* bgFrame = nsCSSRendering::FindNonTransparentBackgroundFrame(mFrame); NS_ASSERTION(bgFrame, "Cannot find NonTransparentBackgroundFrame."); nscolor bgColor = bgFrame-> GetVisitedDependentColor(&nsStyleBackground::mBackgroundColor); nscolor defaultBgColor = mPresContext->DefaultBackgroundColor(); mFrameBackgroundColor = NS_ComposeColors(defaultBgColor, bgColor); GetVisitedDependentColor is also used to ensure contrast of foreground colors on OSX.

Oh lovely, the selection background color can sometimes have an alpha component. That certainly makes the test-cases easier to write :)

Alright, I believe I have a patch ready for review, I'm just waiting on this try run to complete its OSX tests so I know whether there are any more issues to fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3a191d1da36df46a2a952994e907670f2161337 Fingers crossed that try will run those tests soon!

Whew. This took longer than expected. As mentioned, I managed to get the patch working, by starting from the obvious place: having it call the same color-inverting/tweaking code that's later in the same function that mats pointed out in comment 38. With that, things were nearly working, even on OSX. The major problem was that a web platform test, webkit-text-fill-color-property-002.html, was failing because it was testing that both of these rules had the same color when their matching element's text was selected: p { -webkit-text-fill-color:green; } p { color:green; } But since my ua.css changes didn't include a default -webkit-text-fill-color, the text for that case remained green when selected. That was a trivial fix, just adding the requisite line to ua.css. Unfortunately, that broke using the regular "color" property in moz-selection. That is, rules like the following were now ignored, and the default -webkit-text-fill-rule in the UA sheet was used instead for the color: p::moz-selection { color:green; } That's because the code did this to find the text-color: mSelectionTextColor = sc->GetVisitedDependentColor(&nsStyleText::mWebkitTextFillColor); That is, it wasn't considering the "color" property, just the -webkit-text-fill-color that I was now providing in ua.css. Thankfully, the fix for that wasn't too tough. I just had to change that code to this: mSelectionTextColor = nsRuleNode::HasAuthorSpecifiedRules(sc, NS_AUTHOR_SPECIFIED_COLOR, true) ? sc->GetVisitedDependentColor(&nsStyleColor::mColor) : sc->GetVisitedDependentColor(&nsStyleText::mWebkitTextFillColor); In other words, if the ::selection has an author-specified "color", use that. If not, fall back on whatever we normally would use. This did the trick, and since we're already using HasAuthorSpecifiedRules in the same function to get the text-shadow, I suspect it should be fine to take this approach. It did of course mean that I had to actually implement NS_AUTHOR_SPECIFIED_COLOR, since that wasn't an option yet. As such, this new version of that patch has these changes: - adding a default -webkit-text-fill-color to my other ua.css changes. - adding support for NS_AUTHOR_SPECIFIED_COLOR. - using it to choose whether to use the "color" or "-webkit-text-fill-color". - always doing the same color-tweaking that's done at the end of the function, to make sure colors are inverted if they're similar as we're doing pre-patch. - adding new tests to verify that selection colors are inverted appropriately. - making sure that alpha components are properly handled in the tests, since they're used by the default selection colors in some look-and-feels. - adding an OSX-specific version of the new reftest file for coverage there as well (since OSX handles foreground colors differently). - modified a devtools mochitest to expect 13 rules now instead of 12 in ua.css. - fixing the #define indentation/etc issues raised in comment 34. I'm running a new try run here just in case, but I think this is ready for review. (Though if you'd like me to split the NS_AUTHOR_SPECIFIED_COLOR bits out into a separate patch, I don't mind doing so.) The new try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=406f5ec8106f81d444bad411c38d3aa61f22e40f

Comment on attachment 8849842 [details] [diff] [review] 234102-add_default_moz_selection_which_uses_moz_specific_colors.diff Ugh, nope. There are still try failures. Hopefully they're minor, but I'm cancelling the review for now just in case.

I'm a bit confused by the new proposed behavior in terms of when EnsureDifferentColors will happen. What cases will it happen in after the patch? What cases does it happen in now?

To be honest, the more I try to figure this out, the more puzzling it becomes (I think I may be in over my head). Even disregarding the EnsureDifferentColors/color-inverting issue, there is a more fundamental problem somewhere here related to -webkit-text-fill-color and regular color. When I only add a default "::-moz-selection { color:x }" rule to ua.css, things seem to work (of course discounting the color-inversion issues). However, the web platform test webkit-text-fill-color-property-002.html breaks. That test does this: p { -webkit-text-fill-color:green; } I would expect that selecting the text for a p in that case would use the colors in ua.css, but instead it uses the green, and I can't quite fathom why it would do that. I suspected that just meant I should also add a default -webkit-text-fill-color rule to ua.css, in addition to a color. But that breaks simple ::-moz-selection rules instead, if they only specify a color instead of a -webkit-text-fill-color: ::-moz-selection { color:green; } Rather than picking the green from that rule, the engine now picks the default -webkit-text-fill-color that I added to the ua.css to fix the previous problem. This is the code that it's using to determine the color in nsTextPaintStyle::InitSelectionColorsAndShadow: sc = mPresContext->StyleSet()-> ProbePseudoElementStyle(selectionElement, CSSPseudoElementType::mozSelection, mFrame->StyleContext()); mSelectionTextColor = sc->GetVisitedDependentColor(&nsStyleText::mWebkitTextFillColor); For whatever reason, GetVisitedDependentColor is returning the wrong color in both cases. I tried simply having the above code prefer "color" instead of -webkit-text-fill-color, if the style context has it defined. But of course that just chooses the wrong color in a case like this: ::-moz-selection { color:red; -webkit-text-fill-color:green; } So I'm basically at a lost right now. I'm going to look at the patches that added -webkit-text-fill-color in the hopes that they'll shed some light on this (https://bugzilla.mozilla.org/show_bug.cgi?id=1247777). I'd rather revisit the color-inversion cases again with fresh eyes once this mess is untangled.

Hmm. Sorry for yet another wall of text, but this time I definitely need some input :) I did some tracing to determine what's going on right now with the related code in moz-central, with the only change I made being the addition of a default ::moz-selection style to ua.css. The results are a bit confusing. First off, the code being effectively run in nsTextPaintStyle::InitSelectionColorsAndShadow to get the text selection color is this: RefPtr<nsStyleContext> sc = nullptr; sc = mPresContext->StyleSet()-> ProbePseudoElementStyle(selectionElement, CSSPseudoElementType::mozSelection, mFrame->StyleContext()); mSelectionTextColor = sc->GetVisitedDependentColor(&nsStyleText::mWebkitTextFillColor); Where that GetVisitedDependentColor call ultimately runs this: mSelectionTextColor = sc->StyleColor()->CalcComplexColor(sc->StyleText()->mWebkitTextFillColor); In other words, it's calling CalcComplexColor: nscolor CalcComplexColor(const mozilla::StyleComplexColor& aColor) const { return mozilla::LinearBlendColors(aColor.mColor, mColor, aColor.mForegroundRatio); } As such, I changed CalcComplexColor to output the values it sees for aColor.mColor, mColor and aColor.mForegroundRatio. Similarly, I made sure to output the color values in the nsStyleContext that's returned by ProbePseudoElementStyle(mozSelection): sc_wkcolor = sc->GetVisitedDependentColor(&nsStyleText::mWebkitTextFillColor); sc_color = sc->GetVisitedDependentColor(&nsStyleColor::mColor); sc_bgcolor = sc->GetVisitedDependentColor(&nsStyleBackground::mBackgroundColor); Then I made a trivial test-page that only has this rule: p { -webkit-text-fill-color: green; } That way when text is selected on that page it shouldn't be green, but rather whatever color the ua.css says a ::-moz-selection should have. I then set that value in ua.css with this rule: *|*::-moz-selection { -webkit-text-fill-color: red; color: yellow; background-color: blue; } And I got these results for selected text when I ran a test, as one would expect (red selection text): aColor.mColor = red mColor = yellow aColor.mForegroundRatio = 0 sc_wkcolor = red sc_color = yellow sc_bgcolor = blue However, it should still end up using the ::-moz-selection color even without that -webkit-text-fill-color rule in ua.css. So I tried that: *|*::-moz-selection { color: yellow; background-color: blue; } This time, the expected text color would of course be yellow. But we get green instead: aColor.mColor = green mColor = yellow aColor.mForegroundRatio = 0 sc_wkcolor = green sc_color = yellow sc_bgcolor = blue So it seems as though the nsStyleContext returned by ProbePsuedoElement(mozSelection) is falling back on the non-pseudo -webkit-text-fill-color of the text, rather than using the color of the ua.css -moz-selection. Is this possibly a bug? I know the ::selection spec says that [1]: >If color is not specified, the text (and text decoration)'s unselected color must be used for the highlighted text. However, I did specify a color in my ua.css, just not a -webkit-text-fill-color specifically. What do you think, :bz? I'm not sure if this is how things *should* be working just from the info I've read in bug 1247777. And if it is, I'm not sure what I should do about this since (as I mentioned previously) it's causing a seemingly-correct test to break if we use this "ua.css default selection rules" approach (and just adding a default -webkit-text-fill-color will break other seemingly-correct tests). [1] https://drafts.csswg.org/css-pseudo-4/#issue-6ca196f5

-webkit-text-fill-color is a property that inherits by default in our implementation. So if you have: <div style="-webkit-text-fill-color: blue; color: purple"><span style="color: yellow" /></div> Then the span will have a computed color of "yellow" and a computed -webkit-text-fill-color of "blue". That's what you see happening: the ::selection style for an element inherits from that element, and hence picks up its -webkit-text-fill-color. Now the _initial_ value of -webkit-text-fill-color is currentColor, which means "use the value of 'color'". So if nothing set it, it behaves just like 'color'. The spec for webkit-text-fill-color is at https://compat.spec.whatwg.org/#the-webkit-text-fill-color and defines it as inherited and with an initial value of currentColor, which is what we are doing. I'm a little confused about what exactly failed when you had a UA ::selection style that specified color but not -webkit-text-fill-color. Why was that a problem, exactly? Did it change the rendering of testing/web-platform/tests/compat/webkit-text-fill-color-property-002.html or of the reference, and in what way?

Yes, the problem is that the rendering of testing/web-platform/tests/compat/webkit-text-fill-color-property-002.html changes (not the reference), just by adding this to ua.css: *|*::-moz-selection { color: HighlightText; background-color: Highlight; } You can see the results in the attached screenshot; it's using the test's green -webkit-text-fill-color for the selection text color, rather than "HighlightText". As mentioned, adding an extra "-webkit-text-fill-color: HighlightText" fixes that issue, but throws off other tests instead.

> it's using the test's green -webkit-text-fill-color for the selection text color Shouldn't it always do that? At least on Mac, the selection text color is green on that test, without any changes to ua.css. Or is that just because on Mac the color of a selection doesn't change? In any case, that test looks odd to me, unless it's assuming some _very_ specific ::selection styles, and I'm not clear on which ones those would be. Looks like it was written by Jeremy; pinging him....

>Or is that just because on Mac the color of a selection doesn't change? Yes, OSX doesn't change the foreground color of selected text (except to invert it for readability). On Linux and Windows however, you'll see the results I showed in the screenshot, and the WPT will accordingly fail.

Well, the test (webkit-text-fill-color-property-002.html) is for checking that `-webkit-text-fill-color` has the same effect as `color` in the reference. The pass condition is as listed "Pass if color of selected text is green or inverted", where the "inverted" word is for windows/linux platforms. The reason is, we invert the selected text to white (discarding its specified color) on Windows/Linux to make it align with its OS behavior.

> Well, the test (webkit-text-fill-color-property-002.html) is for checking that `-webkit-text-fill-color` has the same effect as `color` in the reference. That seems to be assuming things about ::selection styles which may not be true. And aren't, per the discussion in this bug. Is that test passed by any other browsers on non-Mac platforms?

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #58) > > Well, the test (webkit-text-fill-color-property-002.html) is for checking that `-webkit-text-fill-color` has the same effect as `color` in the reference. > > That seems to be assuming things about ::selection styles which may not be > true. And aren't, per the discussion in this bug. > > Is that test passed by any other browsers on non-Mac platforms? IIRC, I did checked all these tests could pass by Edge on Windows and Chrome on Linux first, then wrote these tests. I don't have a windows machine around at the moment, so I just did a quick verification on my local Linux machine. The result shows that this exact test (webkit-text-fill-color-property-002.html) indeed passed by Chrome 57 on Ubuntu 16.04.

OK, so I just tested what Chrome on Windows actually does with -webkit-text-fill-color. What it does is that it ignores the -webkit-text-fill-color styling for the selection _unless_ there is a ::selection style setting the color or background-color. Then it uses the -webkit-text-fill-color for the selection color. This behavior is obviously incompatible with having the default selection be defined via ::selection. :(

Oh gross. Maybe we should we push them and others to improve that behavior? I can't see existing usage of -webkit-text-fill-color combined with ::selection being terribly high, given that -webkit-text-fill-color is used by ~2% of sites to begin with: https://www.chromestatus.com/metrics/css/timeline/popularity/321 If not I guess it's back to the drawing board.

Well. The spec draft at https://drafts.csswg.org/css-pseudo-4/#highlight-styling says: The UA should use the OS-default highlight colors when neither color nor background-color has been specified by the author. and has a Note: Note: This paired-cascading behavior does not allow using the normal cascade to represent the OS default selection colors. However it has been interoperably implemented in browsers and is thus probably a Web-compatibility requirement. So I think the right fix for this bug is probably to implement that, and the spec should add a note that having ::selection in UA stylesheet is not allowed or something...

But note https://github.com/w3c/csswg-drafts/issues/374

I also filed https://github.com/w3c/csswg-drafts/issues/1122