Closed Bug 1229979 Opened 9 years ago Closed 7 years ago

Multi-color shadow shouldn't be allowed when the color of text is different to the color of decoration line.

Categories

(Core :: Layout: Text and Fonts, defect, P3)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: kuoe0.tw)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

testcase
341 bytes, text/html
Details
Bug 1229979 - (Part 1) Make the color of decoration line shadow be as same as the color of the text shadow.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1229979 - (Part 2) Update reference of test case.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
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.
Attached file testcase — 
Forgot to attach the testcase...
Blocks: css-text-decor-3
I'll try to fix this bug.
Assignee: nobody → kuoe0
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.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
(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.
Status: NEW → ASSIGNED
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
Summary: The shadow of decoration line is draw in a different color when the shadow blur is not zero → The color of the shadow of the decoration line should be as same as the color of the shadow of the text.
Summary: The color of the shadow of the decoration line should be as same as the color of the shadow of the text. → Multi-color shadow shouldn't be allowed when the color of text is different to the color of decoration line.
Attachment #8905347 - Flags: review?(jfkthame)
Attachment #8905348 - Flags: review?(jfkthame)
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
Attachment #8905347 - Flags: review?(jfkthame) → review+
Comment on attachment 8905348 [details]
Bug 1229979 - (Part 2) Update reference of test case.

https://reviewboard.mozilla.org/r/177142/#review182280
Attachment #8905348 - Flags: review?(jfkthame) → review+
Priority: -- → P3
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
Flags: needinfo?(kuoe0)
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.
Flags: needinfo?(kuoe0)
Attachment #8905348 - Flags: review+ → review?(jfkthame)
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.
Attachment #8905348 - Flags: review?(jfkthame) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: