1267128 - -webkit-background-clip: text does not respect -webkit-text-stroke property

Status: RESOLVED DUPLICATE of bug 1269971

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

Description

[Jeremy Chen ([e-mail])]

Attachment: testcase-gradient-stroke.html

After webkit-background-clip: text (759568) and webkit-text-stroke (1248708) landed, it seems that the former property does not respect the latter one. Try open the attached testcase in chrome, the text would be shown w/ gradient stroke, whereas in nightly, the text would be shown nearly nothing.

A screenshot is attached to show the difference between FF nightly (left part) and Chrome (right part).

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: screenshot

A screen shot to show the difference between FF nightly (left part) and Chrome (right part).

Comment 2

[Jeremy Chen ([e-mail])]

Hi Mike, according to https://compat.spec.whatwg.org/#the-webkit-background-clip-property, it seems not clear that whether we should let webkit-background-clip: text respect webkit-text-stroke. Would you mind raise a issue for this? IMHO, the behavior in Chrome seems make sense.

Comment 3

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> Hi Mike, according to
> https://compat.spec.whatwg.org/#the-webkit-background-clip-property, it
> seems not clear that whether we should let webkit-background-clip: text
> respect webkit-text-stroke. Would you mind raise a issue for this? IMHO, the
> behavior in Chrome seems make sense.

Filed https://github.com/whatwg/compat/issues/52 for this.

Comment 4

[Mike Taylor [:miketaylr]]

Thanks for filing an issue Jeremy -- I agree Chrome's behavior is probably what we want (and what existing websites are expecting).

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: testcase-2: stroking by -webkit-background-clip: text

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: testcase-2-ref: stroking by -webkit-text-stroke

Comment 7

[Jeremy Chen ([e-mail])]

At first, I thought we would like to see the stroking results in [1] and [2] are exactly match. However, after verified on webkit and blink, things do not go this way.

Just discussed w/ xidorn offline, it turns out that [1] and [2] should not be the same. In [2], text is stroked after being filled, so a complete stroke is shown. Whereas in [1], the -webkit-text-stroke-color is set to transparent, so the inner stroke would be filled w/ white (as -webkit-text-fill-color) and the outer stroke would be filled w/ background-color.

I'll find another way to add tests then.


[1] attached: testcase-2: stroking by -webkit-background-clip: text
[2] attached: testcase-2-ref: stroking by -webkit-text-stroke

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: part1: support stroke-width while copying glyph to path builder.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: part2: add reftest.

Comment 10

[Jeremy Chen ([e-mail])]

Looks like I might have to take cairo back-end into consideration as well. Clear review requests first.

Comment 11

[u459114]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> Looks like I might have to take cairo back-end into consideration as well.
> Clear review requests first.

On which platform?

Comment 12

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #11)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> > Looks like I might have to take cairo back-end into consideration as well.
> > Clear review requests first.
> 
> On which platform?

Looks like only MacOS go w/ skia backend. Linux and Windows both go w/ cairo. :/
Here is the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=298f53b1fba7

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8745885 [details] [diff] [review]
part1: support stroke-width while copying glyph to path builder.

In this patch, instead of copying the pure glyph path to PathBuilder, I'd like to copy an enlarged one (enlarged by adding stroke info to path). It works for skia-backend; however, it doesn't work for cairo-backend. It turns out that cairo doesn't support stroke_to_path function (see [1]).

AFAIK, we're planning to enable skia by default. I don't think it's worth to implement stroke_to_path function for cairo just to fix this bug, since adding such a function also need lots of tests for the functionality.

Hi jfkthame, do you have any idea how to fix this?

[1] https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/gfx/cairo/cairo/src/cairo.c#2127-2141

Comment 14

[Jonathan Kew [:jfkthame]]

I'm not sure how close we are to switching completely away from cairo; it would seem bad to be in a state where we get significantly different results from background-clip:text on different systems.

Can we do something based on the code in _cairo_recording_surface_get_path without too much effort? From a quick glance at [1], it seems like it should be possible to implement this using a couple of internal cairo functions; might be worth trying, at least.

Probably better to check with someone like :jrmuizel, both about using cairo internals to create a workaround here (would he r+ a patch that added a moz_cairo_stroke_to_path function to cairo.c, or something like that?), and about the status of moving to skia (and whether we need to fix this for cairo or not).

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-recording-surface.c?case=true&from=_cairo_recording_surface_get_path#736

Comment 15

[Daniel Holbert [:dholbert]]

<tangent>
If it's at all possible to come up with a safe patch that's backportable to Aurora here, that would be amazing!  This is currently the only blocker for shipping webkit prefix support[1] (though we may discover others as time goes on).  So if we end up with a safe/targeted patch here that we're comfortable backporting to Aurora, then we could let background-clip:text and webkit-prefixed-CSS support ride the "version 48" train all the way to release.

It's sounding a bit like that may not be possible, though.  A cairo-->skia transition would be too risky to backport. (not sure yet if the workaround hinted at in comment 14 would also be too risky).  So: not a big deal, but something to keep in mind when reasoning about possible fixes here.
</tangent>

* (the webkit prefix support "ride-the-trains" bug is bug 1259345, which this blocks indirectly via bug 1264905.)

Comment 16

[Jeff Muizelaar [:jrmuizel]]

I'm a bit confused by what's going on in this bug. Can someone explain what functionality you're trying to get at?

WebKit has a cairo backend which presumably supports this functionality. Can we not use the same approach used there?

Comment 17

[Daniel Holbert [:dholbert]]

(I gave Jeff the backstory in #gfx.)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> WebKit has a cairo backend which presumably supports this functionality. Can
> we not use the same approach used there?

This is an interesting question.  (My local testing shows that epiphany-browser on Ubuntu renders the testcase correctly, too, which jrmuizel says is confirmation that WebKit-with-Cairo supports this correctly.)

ALSO, one more data point:  Microsoft Edge 13 seems to have this *exact same bug* -- they render the attached testcase (and the bloomberg article* from bug 1248644) exactly like current Nightly does, with virtually-unreadable pullquotes (very faint ghosts of outlines of the text).  (Perhaps that means we should reevaluate whether this blocks webkit support; I think it probably should still block, since it still is effectively a regression on that bloomberg article & perhaps on other sites. So, it's probably better to err on the side of not breaking things.)

* http://www.bloomberg.com/news/features/2016-02-15/missing-malaysia-jet-mh370-weeks-away-from-keeping-secrets-forever

Comment 18

[Daniel Holbert [:dholbert]]

(Actually, Edge doesn't have this exact same bug after all -- they simply don't support -webkit-text-stroke at all. Which still means they misrender the bloomberg article from bug 1248644.)

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

It seems WebKit uses mask for background-clip: text [1], so it is not an issue for them.

[1] https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp?rev=200394#L602

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

And that also explains why Chrome handles emoji in a somehow more sensible way (though not the ideal way) then us.

Comment 21

[Jeremy Chen ([e-mail])]

dholbert, thank you briefing the situation at #gfx. According to the discussion so far, I'll study the approach about using mask for background-clip: text.

In the meantime, I think we may need to consider 1) using mask only for cairo? or 2) using mask for both cairo and skia? Cause using an additional mask blending could be less efficient than just clip the background context by text path.

Comment 22

[u459114]

In bug 1269971, I switch bg-clip:text implementation from glyph path to mask. Testing result looks good, background image draws on both text and stroke shape.

Comment 23

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #22)
> In bug 1269971, I switch bg-clip:text implementation from glyph path to
> mask. Testing result looks good, background image draws on both text and
> stroke shape.

Nice! \o/

Comment 24

[Jeremy Chen ([e-mail])]

Per comment 22, bg-clip:text implementation is switched from glyph path to mask in Bug 1269971. It can be seen from Bug 1269971 part4 patch, -webkit-text-stroke has been honored. I've verified this on my local as well.

Appreciate CJ's stepping forward. Close this bug.