This is needed for rendering of video captions.
Created attachment 302149 [details] Patch
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.
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
Created attachment 302250 [details] Patch
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.
Created attachment 302253 [details] Patch
(In reply to comment #6) > Created attachment 302253 [details] > Patch Thanks for reviewing! Updated patch.
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
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
Created attachment 302292 [details] Patch
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?
rdar://problem/30583872
Committed <https://trac.webkit.org/changeset/212808>
*** Bug 183077 has been marked as a duplicate of this bug. ***