Apply SVG paint-order to text-decorations

Apply SVG paint-order to text-decorations

BUG=360571
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171070

[fs, 2014-04-08 13:44:45]

Seems the F2F is just too much fun...

[Stephen Chennney, 2014-04-08 14:14:31]

LGTM. I presume the spec says that text decoration is still painted after text,
or something to that effect. Maybe add a comment in the CL indicating any
related spec.

The alternative, that the decoration is considered an integral part of the text,
would seem to make things a lot more complex for us.

[pdr., 2014-04-08 14:20:57]

LGTM with a nit and a suggestion

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right):

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m"
refX="5" refY="5" viewBox="0 0 10 10" overflow="visible">
Nit: these markers just make the test more confusing since they aren't
supported.

https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGInlineTextBox.cpp (right):

https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGInlineTextBox.cpp:546: for (int i = 0; i < 3; i++)
{
Question: can we share this with the loop above (in SVGInlineTextBox::paint)?

[Stephen Chennney, 2014-04-08 15:06:10]

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right):

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m"
refX="5" refY="5" viewBox="0 0 10 10" overflow="visible">
On 2014/04/08 14:20:57, pdr wrote:
> Nit: these markers just make the test more confusing since they aren't
> supported.

I considered this but thought they should stay to test that we do not have
problems when they are defined. i.e. if someone modifies the switch or adds new
paint types to the switch.

[fs, 2014-04-08 15:39:00]

On 2014/04/08 14:14:31, Stephen Chenney wrote:
> LGTM. I presume the spec says that text decoration is still painted after
text,
> or something to that effect. Maybe add a comment in the CL indicating any
> related spec.
> 
> The alternative, that the decoration is considered an integral part of the
text,
> would seem to make things a lot more complex for us.

AFAICR the spec says that overline and underline is painted before the glyphs,
while strike-through is painted after. (Painting the decorations as an integral
part would indeed make it more difficult - and if you want that, then using a
clip/mask instead is not too bad a workaround.) I'll look up the spec. section
and reference it.

[fs, 2014-04-08 15:43:47]

On 2014/04/08 15:39:00, fs wrote:
> On 2014/04/08 14:14:31, Stephen Chenney wrote:
> > LGTM. I presume the spec says that text decoration is still painted after
> text,
> > or something to that effect. Maybe add a comment in the CL indicating any
> > related spec.
> > 
> > The alternative, that the decoration is considered an integral part of the
> text,
> > would seem to make things a lot more complex for us.
> 
> AFAICR the spec says that overline and underline is painted before the glyphs,
> while strike-through is painted after. (Painting the decorations as an
integral
> part would indeed make it more difficult - and if you want that, then using a
> clip/mask instead is not too bad a workaround.) I'll look up the spec. section
> and reference it.

I see that SVGInlineTextBox::paint() already has a "Spec: ..." not to that
effect. IMHO opinion it feels more suitable there (where the order of painting
the decorations/text is). Maybe the paintDecoration() method could/should move
closer to paint() though.

[fs, 2014-04-08 15:50:23]

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
File LayoutTests/svg/paintorder/paintorder-text-decorations.svg (right):

https://codereview.chromium.org/225023029/diff/1/LayoutTests/svg/paintorder/p...
LayoutTests/svg/paintorder/paintorder-text-decorations.svg:3: <marker id="m"
refX="5" refY="5" viewBox="0 0 10 10" overflow="visible">
On 2014/04/08 15:06:10, Stephen Chenney wrote:
> On 2014/04/08 14:20:57, pdr wrote:
> > Nit: these markers just make the test more confusing since they aren't
> > supported.
> 
> I considered this but thought they should stay to test that we do not have
> problems when they are defined. i.e. if someone modifies the switch or adds
new
> paint types to the switch.

Yes, that was my thinking - although I didn't think too much, but rather mostly
just copied the paintorder-text.svg TC - so I blame ed for doing any thinking.
It did seem reasonable to have a marker just in case (the unlikely) disaster
strikes though (i.e, "what schenney said").

https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGInlineTextBox.cpp (right):

https://codereview.chromium.org/225023029/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGInlineTextBox.cpp:546: for (int i = 0; i < 3; i++)
{
On 2014/04/08 14:20:57, pdr wrote:
> Question: can we share this with the loop above (in SVGInlineTextBox::paint)?

No, not as long as the stroke+fill for the decorations (under/overline) should
be painted separately.
(I.e. "fill deco.", "stroke deco.", "fill glyphs", "stroke glyphs" vs. "fill
deco.", "fill glyphs", "stroke deco.", "stroke glyphs")

[fs, 2014-04-08 16:19:34]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-08 16:19:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/225023029/1

[commit-bot: I haz the power, 2014-04-08 16:42:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 16:42:34]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[fs, 2014-04-08 17:05:55]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-08 17:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/225023029/1

[commit-bot: I haz the power, 2014-04-08 17:46:00]

Change committed as 171070