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

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: xidorn, Assigned: kuoe0.tw)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

4 years ago
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.
Reporter

Comment 1

4 years ago
Posted file testcase
Forgot to attach the testcase...
Reporter

Updated

4 years ago
Assignee

Comment 2

3 years ago
I'll try to fix this bug.
Assignee: nobody → kuoe0
Assignee

Comment 3

3 years ago
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)
Reporter

Comment 4

3 years ago
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)
Assignee

Comment 5

3 years ago
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)
Assignee

Comment 6

3 years ago
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
Reporter

Comment 7

3 years ago
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?
Reporter

Comment 9

3 years ago
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
Assignee

Comment 11

2 years ago
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.
Assignee

Updated

2 years ago
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.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8905347 - Flags: review?(jfkthame)
Attachment #8905348 - Flags: review?(jfkthame)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
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 19

2 years ago
mozreview-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

Comment 20

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 24

2 years ago
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 25

2 years ago
mozreview-review
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+

Comment 27

2 years ago
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.