This is needed for rendering of video subtitles.
Created attachment 301028 [details] Patch
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.
Created attachment 301031 [details] Patch
(In reply to comment #3) > Created attachment 301031 [details] > Patch I will add test(s).
Comment on attachment 301031 [details] Patch Removing the "r?" until there are tests.
Created attachment 301339 [details] Patch
Created attachment 301348 [details] Patch
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
Created attachment 301352 [details] Patch
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.
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
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
I'll be happy to review this when EWS is all green.
Created attachment 301482 [details] Patch
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
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
Created attachment 301488 [details] Patch
(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.
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?
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.
rdar://problem/30165746
Created attachment 301607 [details] Patch
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.
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
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.
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
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.
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
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
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
(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?
> > 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.
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.
> > 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.
(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
(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.
Created attachment 301774 [details] Patch
(In reply to comment #37) > Created attachment 301774 [details] > Patch CSS part.
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.
Actually let's agree first on the subset of https://drafts.fxtf.org/paint/ we actually plan to implement here.
Created attachment 301933 [details] Patch
Created attachment 301936 [details] Patch
Comment on attachment 301936 [details] Patch Thanks for reviewing :)
Comment on attachment 301936 [details] Patch Clearing flags on attachment: 301936 Committed r212562: <http://trac.webkit.org/changeset/212562>
All reviewed patches have been landed. Closing bug.