See the attachment. Per spec, if we do not specify color for shadow, it should use the ink color. So in this example, the shadow of the underline should always be green, which is not true when the shadow has any blur.

Forgot to attach the testcase...

I'll try to fix this bug.

After doing some investigation, we found this issue caused by the gfxBlur is an alpha only context. So we can only fill single color (the text color) on the blur shadow. There are ways to fix it: 1. Create two blur shadows with two DrawTarget for text and underline independently, and fill different colors on them. 2. Create one DrawTarget and paint text shadow and underline shadow on it with different colors independently. 3. Create a new RGB blur class to handle the case that text color and decoration color are different.

It sounds like the second way should be the simplest. We have already drawn text and decoration line separately, no? Otherwise we shouldn't be able to draw them with different color. So drawing them independently for shadow shouldn't be too hard, I suppose.

There is the current flow: 1. Draw text and underline on a temp surface (alpha-only). (http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#6236-6247) 2. Paint the blur result on the webpage context with text color. (http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#6249) I think the first way is the simplest. We can do the above flow for text-only and underline-only independently. So, we could do `PaintOneShadow` twice for it, one for text and one for underline. The second way would be more complicated. But there is a issue with the first way and the second one. That is the color of the overlap between the text and the decoration line. I think we should consider the different colors to make the result of blur correct. If consider this issue, maybe the third way is the best.

And whatever use the first way or second way, we have to re-implement the painting order defined in spec[1]. We can't leverage the order[2] we already have in DrawText. [1]: https://www.w3.org/TR/css-text-decor-3/#painting-order [2]: http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#7152-7187

Hmmm, I guess the first way isn't the simplest then, nor the second way. A problem for the second way is that, there could be multiple decoration lines at the same time, and they can have different colors, so you would need to create lots of independent context. I guess it is probably better to have a class handle RGB bluring, so the thrid way sounds to be promising. But I'm not familiar with gfx code, so I have no idea how difficult that could be.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #0) > See the attachment. Per spec, if we do not specify color for shadow, it > should use the ink color. I wonder if that's really the appropriate behavior, or if we should query this in the spec? It seems to imply, for instance, that a colored emoji glyph would by default (i.e. no explicit shadow color is specified) cast a multi-colored shadow, as its ink is multi-colored. But ISTM that a "shadow" is inherently monochrome, regardless of the color of the thing being shadowed. In the case of text with a colored underline (perhaps even multiple, differently-colored decoration lines), I think the natural interpretation of text-shadow is that it causes the complete (decorated) text rendering to cast a single, monochromatic shadow, and not that it causes the (bare) text to cast a shadow that then has various-colored decorations applied to it. Tommy, Xidorn: wdyt? Should we raise this with the CSS WG and propose a change to the spec, or proceed to try and implement what the spec currently says?

I guess that probably makes sense. I'd suggest we raise this issue with CSSWG and see what others think. If we want to unify the color of shadow, the title should be changed to, "... when the shadow blur is zero". We are currently inconsistent to ourselves, which is bad either way I think.

Yeah, the inconsistency is clearly bad, and we should fix it; I just don't want us to put the effort into fixing it in a way that doesn't really make sense! I've filed https://github.com/w3c/csswg-drafts/issues/942 to raise this.

According to the resolution of the issue[1], CSSWG decide to use the currentColor for the shadow and not allow the multi-color shadow. So, the shadow with multi-color in the test case would be wrong. [1]: https://github.com/w3c/csswg-drafts/issues/942

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=290a039e9c5fa0ed1e3f046cb2ecbb683812143e

Comment on attachment 8905347 [details] Bug 1229979 - (Part 1) Make the color of decoration line shadow be as same as the color of the text shadow. https://reviewboard.mozilla.org/r/177140/#review182278

Comment on attachment 8905348 [details] Bug 1229979 - (Part 2) Update reference of test case. https://reviewboard.mozilla.org/r/177142/#review182280

Pushed by tokuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/274175ce0863 (Part 1) Make the color of decoration line shadow be as same as the color of the text shadow. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/2f3ff7517e84 (Part 2) Update reference of test case. r=jfkthame

Backed out for failing reftest layout/reftests/text-shadow/decorations-multiple-zorder.html, at least on Windows and Android: https://hg.mozilla.org/integration/autoland/rev/d4ec10bfab85ccc2bbfd1e7c4424a43dcef97410 https://hg.mozilla.org/integration/autoland/rev/5ebab02707d963ead832dfc491024522ad9937a5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2f3ff7517e8448d3e3bc7434fcaaddd8006c9330&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129459915&repo=autoland > REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/text-shadow/decorations-multiple-zorder.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/text-shadow/decorations-multiple-zorder-ref.html | image comparison, max difference: 63, number of differing pixels: 243

Comment on attachment 8905348 [details] Bug 1229979 - (Part 2) Update reference of test case. Hi jfkthame, could you review the test case again? I updated the color for the overlap texts to prevent some additional pixels appearing.

Comment on attachment 8905348 [details] Bug 1229979 - (Part 2) Update reference of test case. https://reviewboard.mozilla.org/r/177142/#review183274 r=me, provided tryserver confirms that it doesn't fail any more.

try for all: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83912ce729b5169ea7561be4ef3bdecc782d75e6 try for Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebfec4f3f3b77d673642cd563df3a703b4441ed0

Pushed by tokuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/861ff7ec8bd4 (Part 1) Make the color of decoration line shadow be as same as the color of the text shadow. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/aed45fc62262 (Part 2) Update reference of test case. r=jfkthame

https://hg.mozilla.org/mozilla-central/rev/861ff7ec8bd4 https://hg.mozilla.org/mozilla-central/rev/aed45fc62262