1407955 - Issues with text-shadow in SVG text (selected vs non-selected, fill vs stroke vs clipPath)

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase, SVG text with text-shadow

See testcase: the SVG text renders with colored stroke and fill, as specified in the attributes of the <svg> element, and inherits its font family/weight/size from the body. But it fails to render the text-shadow that was also specified on the body element.

(The fact that these properties are inherited from the body in this example is irrelevant; behavior is the same if they're specified in an inline style attribute on the <text> element itself.)

Safari and Chrome both render the expected shadow on the SVG text.

The fun surprise here is that if you select any part of the text, suddenly the shadow appears (for the whole of the text element).

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Don't skip rendering text-shadow when we're painting SVG text without selection

The example here is fixed by simply removing the \!aParams.callbacks guard that prevents us rendering the shadow in the (no-selection) PaintText path. It looks like this dates back to bug 655877, originally as the separate aCallbacks param, later moved to the PaintTextParams struct. So :heycam, do you know if removing this will break anything? (Existing reftests seem to be fine with it, according to https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b5eb4766f666c114f0bf5ea348aef7f920edebc, so if it does result in problems that implies we're lacking suitable test coverage...)

Comment 2

[Robert Longson [:longsonr]]

bug 889235 contains a reftest why does the reftest from that bug pass and the testcase not?

Comment 3

[Cameron McCormack (:heycam)]

The callbacks are used for two different cases.  One is when we have a <text> inside a <clipPath>, and we need to generate paths for the clip path.  The second case is when we have SVG patterns, strokes, etc.  In this case, where we're still painting the text normally, it does make sense to just go ahead and paint the shadow, and not bother about going through a callback function to do it either.

One thing to check would be that when we're in here due to <clipPath>, that aParams.IsPaintText() is indeed false.  It might be we only set that for background-clip:text.  Can you check that Jonathan?

I guess the bug 889235 testcase works because ShouldRenderAsPath() returns false, since the text has no stroke and uses a solid fill colour, so we don't have a callbacks object.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: improved testcase with shadows on various SVG text styles

Here is a more extensive testcase that renders SVG text with a variety of attributes, and (in all cases) with a text-shadow style in effect.

Currently, the most obvious bug we have is that the text-shadow only appears for text that is rendered with fill (but no stroke).

Doing a "Select All" on the testcase makes shadows appear on all the examples. However, looking at exactly how the shadows are rendered raises further questions:

- When the text has a stroke but no fill, should the shadow be a "solid" shadow as if the text were filled, or should it be a shadow of only the stroke? In both Safari and Chrome, it's the stroke that casts the shadow here.

- What about partially-opaque text: should the shadow adopt the same opacity as the element? Chrome and Safari do that for SVG text, though this does not apply to partially-opaque HTML text, which still casts a fully-opaque shadow (in all browsers). The difference between their SVG and HTML behavior seems odd.

- Is is appropriate for text-shadow to apply to text used as a clipPath (i.e. to contribute to the clip region)? It kinda does so when selected, but it looks like we get the position a bit wrong. Both Safari and Chrome do this, though in Safari the positioning also looks wrong (in a different way from ours).

(FWIW, this testcase exhibits painting bugs in both Safari and Chrome, too, so we're not alone in having issues here -- though ours seem the most glaring!)

Comment 5

[Robert Longson [:longsonr]]

• Is is appropriate for text-shadow to apply to text used as a clipPath (i.e. to contribute to the clip region)? It kinda does so when selected, but it looks like we get the position a bit wrong. Both Safari and Chrome do this, though in Safari the positioning also looks wrong (in a different way from ours).

No, it should be the raw geometry only per the specification. Whether it has fill, stroke or shadow should make no difference.

Comment 6

[Robert Longson [:longsonr]]

Attachment: Bug 1407955 - Display text shadow with fill and stroke specified r=jfkthame

Comment 7

[Robert Longson [:longsonr]]

I haven't added any tests for stroke only clipPaths because I think our rendering should probably change to that of Chrome.

Should create a new bug absent the clipPath test to track that but at least we show a shadow of some sort now that doesn't magically appear when you select something.

Comment 8

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=67287356dd36cf49395c9bcedacfc6dc919c76ab

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/624c3df8fcd4
Display text shadow with fill and stroke specified r=jfkthame

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38489 for changes under testing/web-platform/tests

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/624c3df8fcd4

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot