Bug 168044 – Apply SVG styles paint-order, stroke-linejoin, and stroke-linecap on DOM text.

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

[Per Arne Vollan]

This is needed for rendering of video subtitles.

[Per Arne Vollan]

Created attachment 301028 [details]
Patch

[WebKit Commit Bot]

Attachment 301028 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 301031 [details]
Patch

[Per Arne Vollan]

(In reply to comment #3)
> Created attachment 301031 [details]
> Patch

I will add test(s).

[Myles C. Maxfield]

Comment on attachment 301031 [details]
Patch

Removing the "r?" until there are tests.

[Per Arne Vollan]

Created attachment 301339 [details]
Patch

[Per Arne Vollan]

Created attachment 301348 [details]
Patch

[Build Bot]

Comment on attachment 301348 [details]
Patch

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

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

[Per Arne Vollan]

Created attachment 301352 [details]
Patch

[Eric Carlson]

Comment on attachment 301352 [details]
Patch

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

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:442
> +        String css = cssPropertyWithTextEdgeColor(CSSPropertyWebkitTextStroke, strokeWidth(), color, behavior == kMACaptionAppearanceBehaviorUseValue);
> +        css.append(" paint-order: stroke;");
> +        css.append(" stroke-linejoin: round;");
> +        css.append(" stroke-linecap: round;");

A String is immutable to String.append() is extremely inefficient. You should use a StringBuilder for this.

> Source/WebCore/rendering/InlineTextBox.cpp:606
> +    

Nit: please delete this added whitespace (if you use Xcode to edit source files, you should enable automatic whitespace trimming in Preferences -> TextEditing).

> Source/WebCore/rendering/InlineTextBox.cpp:609
> +        

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:616
> +                

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:636
> +    

Ditto.

[Build Bot]

Comment on attachment 301352 [details]
Patch

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

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

[Build Bot]

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

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

[Myles C. Maxfield]

I'll be happy to review this when EWS is all green.

[Per Arne Vollan]

Created attachment 301482 [details]
Patch

[Build Bot]

Comment on attachment 301482 [details]
Patch

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

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

[Build Bot]

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

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

[Per Arne Vollan]

Created attachment 301488 [details]
Patch

[Per Arne Vollan]

(In reply to comment #10)
> Comment on attachment 301352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301352&action=review
> 
> > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:442
> > +        String css = cssPropertyWithTextEdgeColor(CSSPropertyWebkitTextStroke, strokeWidth(), color, behavior == kMACaptionAppearanceBehaviorUseValue);
> > +        css.append(" paint-order: stroke;");
> > +        css.append(" stroke-linejoin: round;");
> > +        css.append(" stroke-linecap: round;");
> 
> A String is immutable to String.append() is extremely inefficient. You
> should use a StringBuilder for this.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:606
> > +    
> 
> Nit: please delete this added whitespace (if you use Xcode to edit source
> files, you should enable automatic whitespace trimming in Preferences ->
> TextEditing).
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:609
> > +        
> 
> Ditto.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:616
> > +                
> 
> Ditto.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:636
> > +    
> 
> Ditto.

Thanks for reviewing :) Updated patch.

[Eric Carlson]

Comment on attachment 301488 [details]
Patch

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

Someone that knows more about text rendering should review this.

> Source/WebCore/rendering/InlineTextBox.cpp:581
> +    case PaintOrderMarkers: {
> +        paintDecorations(paintInfo, lineStyle, context, font, combinedText, textRun, textOrigin, boxRect, boxOrigin, textPaintStyle, textShadow, useCustomUnderlines);
> +
> +        textPainter.paintText(textRun, length, boxRect, textOrigin, selectionStart, selectionEnd, paintSelectedTextOnly, paintSelectedTextSeparately);
> +        break;
> +    }

Nit: are these braces necessary?

> Source/WebCore/rendering/InlineTextBox.cpp:584
> +    case PaintOrderStrokeMarkers: // FIXME: Implement.
> +    case PaintOrderMarkersStroke : // FIXME: Implement.
> +    case PaintOrderFillMarkers : // FIXME: Implement.

Nit: a "FIXME:" should include a bug number.

> Source/WebCore/rendering/InlineTextBox.cpp:591
> +    default: {
> +        textPainter.paintText(textRun, length, boxRect, textOrigin, selectionStart, selectionEnd, paintSelectedTextOnly, paintSelectedTextSeparately);
>  
> -    textPainter.paintText(textRun, length, boxRect, textOrigin, selectionStart, selectionEnd, paintSelectedTextOnly, paintSelectedTextSeparately);
> +        paintDecorations(paintInfo, lineStyle, context, font, combinedText, textRun, textOrigin, boxRect, boxOrigin, textPaintStyle, textShadow, useCustomUnderlines);
> +    }

Nit: are these braces necessary?

[Simon Fraser (smfr)]

Comment on attachment 301488 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=168044

Radar number?

> Source/WebCore/ChangeLog:8
> +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.

Adding support for it in inline text boxes basically means everywhere, right?

You need to reference https://drafts.fxtf.org/paint/ and say how close you are to that spec.

> Source/WebCore/rendering/InlineTextBox.cpp:554
> +    PaintOrder paintOrder = lineStyle.svgStyle().paintOrder();

You need to move these properties out of svgStyle into RenderStyle now that they are no longer SVG-specific, and you need to fix up CSSProperties.json to remove  "svg": true from the "codegen-properties" for these properties.

> Source/WebCore/rendering/InlineTextBox.cpp:556
> +    switch (paintOrder) {

Does InlineTextBox::paintDecoration() have to change too? You should test that thick strokes and clear fills get shadowed correctly, for example.

> LayoutTests/ChangeLog:10
> +        * fast/css/paint-order.html: Added.
> +        * platform/mac/fast/css/paint-order-expected.txt: Added.
> +        * platform/ios-simulator/fast/css/paint-order-expected.txt: Added.

This needs a crapload of ref tests to make sure it works correctly.

[Simon Fraser (smfr)]

rdar://problem/30165746

[Per Arne Vollan]

Created attachment 301607 [details]
Patch

[Build Bot]

Comment on attachment 301607 [details]
Patch

Attachment 301607 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3121705

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301608 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301607 [details]
Patch

Attachment 301607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3121712

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301611 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301607 [details]
Patch

Attachment 301607 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3121710

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301613 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301607 [details]
Patch

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

New failing tests:
imported/mozilla/svg/paint-order-02.svg
imported/mozilla/svg/paint-order-03.svg
fast/css/paint-order.html
imported/blink/svg/paintorder/paintorder.svg
imported/blink/svg/paintorder/paintorder-text.svg

[Build Bot]

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

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

[Per Arne Vollan]

(In reply to comment #20)
> Comment on attachment 301488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301488&action=review
> 

Thanks for reviewing, I will update the patch :)

> > Source/WebCore/ChangeLog:8
> > +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.
> 
> Adding support for it in inline text boxes basically means everywhere, right?
> 

There is also painting of text in SimpleLineLayoutFunctions.cpp (paintFlow), but it seems this is used for text in block elements. According to https://drafts.fxtf.org/paint/, the properties should be applied on inline boxes, but perhaps we also should change the paintFlow function?

[alan]

> > Adding support for it in inline text boxes basically means everywhere, right?
> > 
> 
> There is also painting of text in SimpleLineLayoutFunctions.cpp (paintFlow),
> but it seems this is used for text in block elements.
so as InlineTextBox.
The "inline" part in the InlineTextBox name does not refer to inline-level elements/inline boxes. We also generate InlineTextBoxes for the content of block elements.
This painting logic looks to be at the wrong place and should be moved out of the inline tree context.

>According to
> https://drafts.fxtf.org/paint/, the properties should be applied on inline
> boxes, but perhaps we also should change the paintFlow function?
Currently simple line layout does not support inline elements, so no, you should not change paintflow.

[alan]

Comment on attachment 301607 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.

As per spec, it's not for inline text boxes but inline boxes! This line is misleading as it suggests we need to apply these properties on the content of block elements as well.

[Simon Fraser (smfr)]

> > Source/WebCore/ChangeLog:9
> > +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.
> 
> As per spec, it's not for inline text boxes but inline boxes!

We're actually not sure about this, and are waiting for clarification.

Per, I think you should break this up. First, do the CSS bits, and test with some parsing tests (if there's any behavior change).

Then do the TextPainter changes.

You may also need some new code to handle additional overflow from strokes.

[alan]

(In reply to comment #33)
> Comment on attachment 301607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301607&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.
> 
> As per spec, it's not for inline text boxes but inline boxes! This line is
> misleading as it suggests we need to apply these properties on the content
> of block elements as well.
So thanks to Simon, it's clear now that these properties should applied to all elements. https://twitter.com/tabatkins/status/831991186487287808

[Per Arne Vollan]

(In reply to comment #35)
> (In reply to comment #33)
> > Comment on attachment 301607 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301607&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Add support for paint-order, stroke-linejoin, and stroke-linecap style for inline text boxes.
> > 
> > As per spec, it's not for inline text boxes but inline boxes! This line is
> > misleading as it suggests we need to apply these properties on the content
> > of block elements as well.
> So thanks to Simon, it's clear now that these properties should applied to
> all elements. https://twitter.com/tabatkins/status/831991186487287808

Thanks for clarifying! I will split the patch into a CSS part and a text painter part.

[Per Arne Vollan]

Created attachment 301774 [details]
Patch

[Per Arne Vollan]

(In reply to comment #37)
> Created attachment 301774 [details]
> Patch

CSS part.

[Simon Fraser (smfr)]

Comment on attachment 301774 [details]
Patch

Don't we also need some subset of the other properties in https://drafts.fxtf.org/paint/, like stroke-width, stroke-color etc? Don't just go by the title of this bug.

Also your test needs to a generic CSS test, for example like fast/css/parsing-object-fit.html, and not be captions-specific.

[Simon Fraser (smfr)]

Actually let's agree first on the subset of https://drafts.fxtf.org/paint/ we actually plan to implement here.

[Per Arne Vollan]

Created attachment 301933 [details]
Patch

[Per Arne Vollan]

Created attachment 301936 [details]
Patch

[Per Arne Vollan]

Comment on attachment 301936 [details]
Patch

Thanks for reviewing :)

[WebKit Commit Bot]

Comment on attachment 301936 [details]
Patch

Clearing flags on attachment: 301936

Committed r212562: <http://trac.webkit.org/changeset/212562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.