Bug 168601 – Add support for CSS properties paint-order, stroke-linecap, and stroke-linejoin in text rendering.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

This is needed for rendering of video captions.

[Per Arne Vollan]

Created attachment 302149 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 302149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302149&action=review

Things i think you need to test:

Shadow respects stroke.
What happens to shadow with colored stroke but clear fill.
Shadow painting with non-standard paint-order.

You need to get creative with tests here. Tests with image results aren't useful, because no-one runs pixel tests. You need to create tests that can be ref tests, which you can do by masking/obscuring parts of the text, either with -webkit-clip-path or overlaying divs to just reveal solid blocks of color for the bits you care about.

Code is good, but tests need beefing up.

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

I would like to see some more description here of what you did.

> Source/WebCore/rendering/TextPainter.cpp:133
> +        for (unsigned i = 0; i < paintOrder.size(); ++i) {
> +            switch (paintOrder.at(i)) {

You can use a modern loop here.

> Source/WebCore/rendering/TextPainter.cpp:136
> +                paintTextWithShadows(shadow, *m_font, textRun, boxRect, textOrigin, startOffset, endOffset, nullAtom, 0, false);

Is this going to paint the shadow in each phase? I think that's wrong; you should just paint the shadow once.

> Source/WebCore/rendering/TextPainter.cpp:141
> +                paintTextWithShadows(shadow, *m_font, textRun, boxRect, textOrigin, startOffset, endOffset, nullAtom, 0, paintStyle.strokeWidth > 0);

Same here.

> Source/WebCore/rendering/style/RenderStyleConstants.h:743
> +    PaintOrderNormal = 0,
> +    PaintOrderFill = 1,
> +    PaintOrderFillMarkers = 2,
> +    PaintOrderStroke = 3,
> +    PaintOrderStrokeMarkers = 4,
> +    PaintOrderMarkers = 5,
> +    PaintOrderMarkersStroke = 6

No need to give values if they are contiguous.

[zalan]

Comment on attachment 302149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302149&action=review

> Source/WebCore/rendering/style/RenderStyleConstants.h:736
> +enum PaintOrder {

enum class

> Source/WebCore/rendering/style/RenderStyleConstants.h:746
> +enum PaintType {

enum class

[Per Arne Vollan]

Created attachment 302250 [details]
Patch

[Sam Weinig]

Comment on attachment 302250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302250&action=review

> Source/WebCore/rendering/style/RenderStyleConstants.h:744
> +enum class PaintOrder {
> +    PaintOrderNormal,
> +    PaintOrderFill,
> +    PaintOrderFillMarkers,
> +    PaintOrderStroke,
> +    PaintOrderStrokeMarkers,
> +    PaintOrderMarkers,
> +    PaintOrderMarkersStroke
> +};
> +
> +enum class PaintType {
> +    PaintTypeFill,
> +    PaintTypeStroke,
> +    PaintTypeMarkers
> +};

When using enum class, there is no need to prefix each of the values, since you always use the qualified form.

[Per Arne Vollan]

Created attachment 302253 [details]
Patch

[Per Arne Vollan]

(In reply to comment #6)
> Created attachment 302253 [details]
> Patch

Thanks for reviewing! Updated patch.

[Build Bot]

Comment on attachment 302253 [details]
Patch

Attachment 302253 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3166037

New failing tests:
fast/css/paint-order-shadow.html

[Build Bot]

Created attachment 302261 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Per Arne Vollan]

Created attachment 302292 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 302292 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302292&action=review

> Source/WebCore/rendering/TextPainter.cpp:134
> +        for (auto i : paintOrder) {
> +            switch (i) {

I would not call this i (it's not an index). Call it 'order' or something.

> Source/WebCore/rendering/TextPainter.cpp:144
> +            case PaintType::Stroke:
> +                m_context.setTextDrawingMode(textDrawingMode & ~TextModeFill);
> +                paintTextWithShadows(paintShadow ? shadow : nullptr, *m_font, textRun, boxRect, textOrigin, startOffset, endOffset, nullAtom, 0, paintStyle.strokeWidth > 0);
> +                paintShadow = false;

So if the stroke comes first, is it right to just shadow the stroke?

[Per Arne Vollan]

rdar://problem/30583872

[Per Arne Vollan]

Committed <https://trac.webkit.org/changeset/212808>

[Jon Lee]

*** Bug 183077 has been marked as a duplicate of this bug. ***