Since we've shipped -webkit-text-fill-color in Bug 1247777, selected texts should be rendered w/ -webkit-text-fill-color instead of color.

Why should the color of selected text should be rendered in text-fill-color? It doesn't seem we do that for property color. Chrome doesn't do that for text-fill-color either.

It may make sense to say -webkit-text-fill-color should take effect like color for ::-moz-selection, though.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #2) > It may make sense to say -webkit-text-fill-color should take effect like > color for ::-moz-selection, though. I guess you're just saying that I should re-edit the bug summary, right? Thank you for the noticing and prevent me from resolving this in a wrong way.

Well, I meant it should probably take effect when it is specified in ::-moz-selection pseudo-element. I don't think this is an important issue, though.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4) > Well, I meant it should probably take effect when it is specified in > ::-moz-selection pseudo-element. I don't think this is an important issue, > though. I didn't have time to dig deeper yet. I'm guessing that somewhere in nsTextFrame::GetSelectionTextColors() is not taking -webkit-text-fill-color into consideration. This should fix the problem incluing specifying webkit-text-fill-color in ::-moz-selection pseudo-element as well, I guess. [1] https://dxr.mozilla.org/mozilla-central/rev/17a0ded9bb99c05c25729c306b91771483109067/layout/generic/nsTextFrame.cpp#5650

Except selection, all the rendering parts that -webkit-text-fill-color take effect should be patched as well. I plan to cover those rendering parts in this bug as well. Will start work on this tomorrow. Here's a list of affected rendering parts I found so far: 1. selection text color 2. <math> MathML element color 3. nsDisplayTextOverflowMarker::Paint for TextOverflow 4. nsTextBoxFrame for xul Not sure if item 4 should be patched or not...

Looks like people don't use --manifest-update very often, so I have to update this from time to time. :(

Make it align w/ test file name.

Test whether -webkit-text-fill-color takes effect for selected text, text-decoration, text-overflow, and MathML.

Expect fails on following 5 files: webkit-text-fill-color-property-002.html webkit-text-fill-color-property-003.html webkit-text-fill-color-property-004.html webkit-text-fill-color-property-005.html webkit-text-fill-color-property-006.html The fails have been already verified locally. Let's see how it goes on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14) > Expect fails on following 5 files: > webkit-text-fill-color-property-002.html > webkit-text-fill-color-property-003.html > webkit-text-fill-color-property-004.html > webkit-text-fill-color-property-005.html > webkit-text-fill-color-property-006.html > > The fails have been already verified locally. > Let's see how it goes on try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257 Retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=19335169 No idea why 002 could pass...

Rebased.

Split this from adding test. Make rebase for manifest.json a lot easier.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15) > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14) > > Expect fails on following 5 files: > > webkit-text-fill-color-property-002.html > > webkit-text-fill-color-property-003.html > > webkit-text-fill-color-property-004.html > > webkit-text-fill-color-property-005.html > > webkit-text-fill-color-property-006.html > > > > The fails have been already verified locally. > > Let's see how it goes on try: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257 > > Retry: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=1 > 9335169 > > No idea why 002 could pass... I guess 002 could really pass on Linux, however, it shall not pass on MacOS I think.

After discussing with CJ and Astley offline, CJ could help take care of this bug from here. So, I could get back to Bug 1248708.

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #19) > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #15) > > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14) > > > Expect fails on following 5 files: > > > webkit-text-fill-color-property-002.html > > > webkit-text-fill-color-property-003.html > > > webkit-text-fill-color-property-004.html > > > webkit-text-fill-color-property-005.html > > > webkit-text-fill-color-property-006.html > > > > > > The fails have been already verified locally. > > > Let's see how it goes on try: > > > https://treeherder.mozilla.org/#/ > > > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257 > > > > Retry: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=413a6796bc0ccf66e43409b60c3e9ac61f53d257&selectedJob=1 > > 9335169 > > > > No idea why 002 could pass... > > I guess 002 could really pass on Linux, however, it shall not pass on MacOS > I think. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97119dc845b Assumption proved!! Accordingly, we may need to try MacOS platform to guarantee the patch do work.

Review commit: https://reviewboard.mozilla.org/r/46127/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46127/

Comment on attachment 8740962 [details] MozReview Request: Bug 1261578 - Part 1. Correct text color in selection range; https://reviewboard.mozilla.org/r/46101/#review42625 ::: layout/base/nsLayoutUtils.h:1537 (Diff revision 3) > + // Get a suitable text color property from aCtx. > + static nsCSSProperty GetTextColorProp(nsStyleContext *const aCtx); IMO, a more natural home for this would be as part of nsStyleContext; it doesn't depend on anything else besides the context itself, so it can be a simple inline method in nsStyleContext.h such as: nsCSSProperty TextColorProperty() { return StyleText()->mWebkitTextFillColorForeground ? eCSSProperty_color : eCSSProperty__webkit_text_fill_color; } (See also the existing GetTextFillColor() method; this would logically go alongside it.) Obviously, this will have a knock-on effect on the callsites...

Comment on attachment 8740963 [details] MozReview Request: Bug 1261578 - Part 2. Correct text decoration color; https://reviewboard.mozilla.org/r/46103/#review42635 This basically looks like it should be fine; just a few nits I'd like to get cleaned up, please. ::: layout/generic/nsTextFrame.cpp:410 (Diff revision 3) > free(userData); > } > } > > +static nsCSSProperty > +ChooseDecorationTextColorProp(nsStyleContext *const aCtx) We don't normally use `const` on pointers like this; there's no real benefit here, AFAICS, it just adds clutter. (If we could declare the `nsStyleContext` itself as being `const`, that'd be different -- but we can't do that because accessors like StyleText() are not `const` methods.) ::: layout/generic/nsTextFrame.cpp:412 (Diff revision 3) > + const nsStyleTextReset*const styleTextReset = aCtx->StyleTextReset(); > + const nsStyleText*const styleText = aCtx->StyleText(); Likewise, drop the second `const` from each of these. ::: layout/generic/nsTextFrame.cpp:413 (Diff revision 3) > > +static nsCSSProperty > +ChooseDecorationTextColorProp(nsStyleContext *const aCtx) > +{ > + const nsStyleTextReset*const styleTextReset = aCtx->StyleTextReset(); > + const nsStyleText*const styleText = aCtx->StyleText(); Also, don't look up `StyleText()` here, as it may not be needed, depending what gets returned in `foreground`. Just use `StyleText()` directly in the `return` expression below, as there's only a single use of it. And for that matter, there's no real need for the `styleTextReset` variable either, as it just has the one simple usage; might as well write `StyleTextReset()->GetDecorationColor(....)` directly there. ::: layout/generic/nsTextFrame.cpp:419 (Diff revision 3) > + return (foreground && !styleText->mWebkitTextFillColorForeground) ? > + eCSSProperty__webkit_text_fill_color : > + eCSSProperty_text_decoration_color; The style guide wants us to break lines *before* the operator, so this becomes return (....) ? eCSS.... : eCSS...; (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators) ::: layout/style/StyleAnimationValue.cpp:3306 (Diff revision 3) > - color = aStyleContext->StyleColor()->mColor; > + const nsStyleText *styleText = aStyleContext->StyleText(); > + color = styleText->mWebkitTextFillColorForeground ? > + aStyleContext->StyleColor()->mColor : > + styleText->mWebkitTextFillColor; I think this is equivalent to: color = aStyleContext->GetTextFillColor();

Comment on attachment 8740963 [details] MozReview Request: Bug 1261578 - Part 2. Correct text decoration color; https://reviewboard.mozilla.org/r/46103/#review43619 ::: accessible/base/TextAttrs.cpp:705 (Diff revision 4) > mStyle = textReset->GetDecorationStyle(); > > bool isForegroundColor = false; > textReset->GetDecorationColor(mColor, isForegroundColor); > if (isForegroundColor) > - mColor = aFrame->StyleColor()->mColor; > + mColor = aFrame->StyleContext()->GetTextFillColor(); While you're here, it'd be nice to add the missing braces for the if-statement.

Comment on attachment 8740962 [details] MozReview Request: Bug 1261578 - Part 1. Correct text color in selection range; https://reviewboard.mozilla.org/r/46101/#review43617

Comment on attachment 8741010 [details] MozReview Request: Bug 1261578 - Part 3. Correct MathML text color; https://reviewboard.mozilla.org/r/46117/#review43621

Comment on attachment 8741028 [details] MozReview Request: Bug 1261578 - Part 4. Correct text overflow color; https://reviewboard.mozilla.org/r/46127/#review43623

Comment on attachment 8741029 [details] MozReview Request: Bug 1261578 - Part 5. web-platform-test reftest; https://reviewboard.mozilla.org/r/46129/#review43627 Looks fine, with a couple minor fixups. ::: testing/web-platform/tests/compat/webkit-text-fill-color-property-002.html:17 (Diff revision 2) > + color: red; > + -webkit-text-fill-color: green; > +} > +</style> > +<body onload="onload()"> > +<p>Pass if color of <span id="selectMe">selected text</span> is green!!!</p> Although what this test actually does -- checking that `-webkit-text-fill-color` has the same effect as `color` in the reference -- should be fine, ISTM the phrasing of the "pass" condition here is a problem, because (depending on the platform conventions) we don't necessarily render selected text using the foreground color at all -- on Windows, we invert the text to white (discarding its specified color). Maybe something like "Pass if color of selected text is green or inverted (depending on the platform convention), but not red." Or am I confused about what's happening here? ::: testing/web-platform/tests/compat/webkit-text-fill-color-property-006.html:24 (Diff revision 2) > + font-family: Ahem; > + font-size: 30px; > +} > +</style> > +<body> > +<p>Test passes if there is an <strong>green ellipsis</strong> after "TestChecks".</p> s/an/a/, here and in reference

Comment on attachment 8741652 [details] MozReview Request: Bug 1261578 - Part 6. Before update web-platform-test manifest; https://reviewboard.mozilla.org/r/46665/#review43629 ::: testing/web-platform/meta/MANIFEST.json (Diff revision 1) > - "path": "web-animations/animation-model/keyframes/effect-value-context.html", > - "url": "/web-animations/animation-model/keyframes/effect-value-context.html" > - }, > - { Shouldn't this file still be mentioned somewhere? It looks like it was out-of-place in the list, so I'd expect to see it move, but not disappear altogether.

https://reviewboard.mozilla.org/r/46665/#review43629 > Shouldn't this file still be mentioned somewhere? It looks like it was out-of-place in the list, so I'd expect to see it move, but not disappear altogether. I follow how jeremy update manifest in Bug 1247777 1. run ./mach web-platform-tests --manifest-update without new test cases add 2. add test cases 3. run ./mach web-platform-tests --manifest-update again The result looks like noraml after following these steps.

Review commit: https://reviewboard.mozilla.org/r/47425/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47425/

Comment on attachment 8741652 [details] MozReview Request: Bug 1261578 - Part 6. Before update web-platform-test manifest; https://reviewboard.mozilla.org/r/46665/#review44105

Comment on attachment 8742700 [details] MozReview Request: Bug 1261578 - Part 7. Update web-platform-test manifest for fill-color reftest; https://reviewboard.mozilla.org/r/47425/#review44107

how soon... thanks Jonathan.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e94186bab384 https://hg.mozilla.org/integration/mozilla-inbound/rev/92a26bb2d41c https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc5f3fc9d70 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec793ac0a38 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a3af62e8f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/57fef863d70e https://hg.mozilla.org/integration/mozilla-inbound/rev/979bd96a0dee

https://hg.mozilla.org/mozilla-central/rev/e94186bab384 https://hg.mozilla.org/mozilla-central/rev/92a26bb2d41c https://hg.mozilla.org/mozilla-central/rev/2fc5f3fc9d70 https://hg.mozilla.org/mozilla-central/rev/4ec793ac0a38 https://hg.mozilla.org/mozilla-central/rev/a6a3af62e8f9 https://hg.mozilla.org/mozilla-central/rev/57fef863d70e https://hg.mozilla.org/mozilla-central/rev/979bd96a0dee

Nice job!! \O/ CJ, thank you for taking care of this.

It seems like this requires spec changes, in that it changes the definition of the currentcolor value for text-decoration-color from using 'color' to using '-webkit-text-fill-color'. I'm also curious why it made that change for text-decoration but not text-emphasis. Xidorn, would you be able to deal with sending the appropriate email to www-style to discuss these spec issues, or should I? (This probably should have been done before it landed.)

Also, do all of the different changes (parts 1, 2, 3, and 4) in this bug match the webkit/blink behavior?

I think so. I verified my changes, in Part 1~4, by test cases in part 5, and using those test cases to make sure we have the same behavior with safari & google chrome.

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #69) > It seems like this requires spec changes, in that it changes the definition > of the currentcolor value for text-decoration-color from using 'color' to > using '-webkit-text-fill-color'. > > I'm also curious why it made that change for text-decoration but not > text-emphasis. Attached an example with -webkit-text-fill-color + text-emphasis 1. webkit does not apply text-fill-color to emphasis. 2. blink does not support text-emphasis(unless I am wrong)

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #69) > It seems like this requires spec changes, in that it changes the definition > of the currentcolor value for text-decoration-color from using 'color' to > using '-webkit-text-fill-color'. > > I'm also curious why it made that change for text-decoration but not > text-emphasis. Leaving the needinfo? for text-emphasis. > Xidorn, would you be able to deal with sending the appropriate email to > www-style to discuss these spec issues, or should I? I'll send the email to www-style. > (This probably should have been done before it landed.) I'd like to re-emphasize this. We shouldn't be landing changes that require spec changes without pointing out those spec changes in the appropriate working groups. That's something for both the patch author and reviewer to be paying attention to.

I think we should handle text-emphasis-color and text-decoration-color the same way, as they are both decoration. But I don't think we should change "text-decoration-color: currentcolor" to use the value from -webkit-text-fill-color. I've replied the post in www-style. As I said there, that could make implementing interpolation between currentcolor and non-currentcolor value unnecessarily more complicated.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #76) > I think we should handle text-emphasis-color and text-decoration-color the > same way, as they are both decoration. But I don't think we should change > "text-decoration-color: currentcolor" to use the value from > -webkit-text-fill-color. I've replied the post in www-style. As I said That means some of the changes in this bug need to be backed out.

Hmm, it looks like Safari & Chrome do something rather weird with decorations, actually: if -webkit-text-stroke-width is greater than 0px, they actually paint the decoration using the color from -webkit-text-stroke-color rather than ...fill-color. (But they don't change the width of the line to follow -webkit-text-stroke-width, so they're not entirely treating it as a "stroke".)

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #77) > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #76) > > I think we should handle text-emphasis-color and text-decoration-color the > > same way, as they are both decoration. But I don't think we should change > > "text-decoration-color: currentcolor" to use the value from > > -webkit-text-fill-color. I've replied the post in www-style. As I said > > That means some of the changes in this bug need to be backed out. Here is what I am going to do 1. Back out change in Part 2. 2. "expected:FAIL" in ini files of text decoration test. I think it's better then just remove them. 3. Change string in test case. For example, change "Pass if text underline is green!!!" to "Fail if text underline is green!!!"

(In reply to C.J. Ku[:cjku] from comment #79) > 2. "expected:FAIL" in ini files of text decoration test. I think it's better > then just remove them. > 3. Change string in test case. For example, change "Pass if text underline > is green!!!" to "Fail if text underline is green!!!" I guess it is better just remove that test directly if that was written by you. jgraham, what do you think?

bug 1266948 filed

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #80) > (In reply to C.J. Ku[:cjku] from comment #79) > > 2. "expected:FAIL" in ini files of text decoration test. I think it's better > > then just remove them. > > 3. Change string in test case. For example, change "Pass if text underline > > is green!!!" to "Fail if text underline is green!!!" > > I guess it is better just remove that test directly if that was written by > you. jgraham, what do you think? If the tests are now believed to be wrong i.e. not testing the behaviour in the spec, remove them. If they are right per-spec and our implementation is temporarily wrong in a way that we expect to fix in the future, mark them expected:FAIL. If they are wrong per spec and the correct-per-spec behaviour is not tested elsewhere, fix them to test the correct behaviour. If the spec is wrong and we don't intend to implement it things are a little more complex, but generally editing the tests to match the expected spec is fine if the change is uncontroversial (e.g. just matches the behaviour of existing implementations).

(In reply to James Graham [:jgraham] from comment #82) > If they are wrong per spec and the correct-per-spec behaviour > is not tested elsewhere, fix them to test the correct behaviour. This case. CJ has filed bug 1266948 to change the behavior and the test.