Draw a single underline for text-decoration-style: [dashed, dotted].

Draw a single underline for text-decoration-style: [dashed, dotted].

There is a bug in the new text-decoration styles where the dashed and dotted
underlines would be drawn twice. This appears to be because we are stroking
a SkRect of height zero.

Fixed by drawing a line rather than stroking a rect.

BUG=348518
TEST=Covered by existing tests

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

[andersr, 2014-03-05 14:55:18]

Like I mentioned to abinader on IRC, I have two thoughts regarding this patch:

1) Should I do something with RenderBoxModelObject::shouldAntialiasLines?
Perhaps put GraphicsContext in some new auto mode, instead of letting "the
outside" specify the details explicitly?

2) For drawing the dashed and dotted decorations (more) properly, I use the same
code which is used for drawing borders. This code does some special stuff, like
adjust the phase of the pattern to make the corners look good. This has the
benefit of always drawing something at both ends of the line, but it can look a
bit strange in some cases. See the dashed "IIIII" in the After section of the
attachment [1], which has very short and dot-like dashes at the ends, or the
dotted "II" which has very dash-like dots at the ends. :) Compare with Firefox'
implementation, which appears to simply clip the decoration at the right end.

I'm not sure, but I might be inclined to prefer the worst case of Firefox rather
than Blink's (new) worst case. I could also (now, or in the future) implement
something like background-repeat: space, where we draw N whole dashes, but space
them out to align with the ends as much as possible.

[1] https://dl.dropboxusercontent.com/u/669046/screenshots/text_decoration.png

[Stephen Chennney, 2014-03-05 15:59:34]

This change LGTM.

Regarding the dashing, the GraphicsContext implementation tries to ensure that
there is always a portion of dot or dash at the end of the line and that the
dash/dot spacing is centered in the line, but these two things mix up to
generate some odd results. I do think that, in theory, it is correct, but I have
strong doubts about the implementation. I just didn't take the time 'til now to
investigate fully.

If you're interested, feel free to modify the way the dashes are set up in
StrokeData::setupPaintDashPathEffect. In particular, I think the way it simply
subtracts off the line thickness for the endcap drawing is wrong. It should be
possible to not draw the endcaps at all and instead set the phase to ensure that
there is always a piece of line at each endpoint.

https://codereview.chromium.org/185683003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBoxModelObject.h (right):

https://codereview.chromium.org/185683003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBoxModelObject.h:270: static bool
shouldAntialiasLines(GraphicsContext*);
I'm appalled at the runtime cost of implementing this method. We ask the
Graphics context for the current transform, which means we create an entire
AffineTransform from a SkMatrix, then we figure out what properties the matrix
has. I would love to know how often the answer is false.

[Stephen Chennney, 2014-03-05 16:03:26]

On 2014/03/05 14:55:18, andersr wrote:
> 1) Should I do something with RenderBoxModelObject::shouldAntialiasLines?
> Perhaps put GraphicsContext in some new auto mode, instead of letting "the
> outside" specify the details explicitly?

I don't like the idea of a special mode in GraphicsContext unless it can be made
to only add cost in this specific case. That doesn't seem likely. I would rather
we made some effort to optimize the call to shouldAntialiasLines. The planned
effort to back Blink geometry types, particularly rects and matrices, with Skia
types should help make the query cheaper.

[andersr, 2014-03-05 19:45:57]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-03-05 19:47:19]

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

[andersr, 2014-03-05 20:43:07]

The CQ bit was unchecked by andersr@opera.com

[Stephen Chennney, 2014-03-06 00:50:07]

I now see why I was wrong about it always being possible to start/end any line
with a piece of dash. My feeling is that we should stretch out the spaces
between dashes/dots in that case to remove the end-of-line space.

[andersr, 2014-03-06 12:13:16]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-03-06 12:13:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/185683003/30001

[commit-bot: I haz the power, 2014-03-06 12:49:01]

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

[commit-bot: I haz the power, 2014-03-06 12:49:01]

Try jobs failed on following builders: mac_blink_rel

[Stephen Chennney, 2014-03-06 21:21:13]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-03-06 21:21:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/185683003/30001

[Stephen Chennney, 2014-03-06 21:25:05]

I think this will fail to apply so you might need to rebase.

[commit-bot: I haz the power, 2014-03-06 22:06:59]

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

[commit-bot: I haz the power, 2014-03-06 22:07:00]

Try jobs failed on following builders: mac_blink_rel

[andersr, 2014-03-07 09:24:08]

> I think this will fail to apply so you might need to rebase.

Rebased already, so it applies now, but there are some unexpected fails I must
look into. Probably related to the anti-aliasing changes.

[andersr, 2014-03-07 13:54:19]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-03-07 13:54:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/185683003/60005

[commit-bot: I haz the power, 2014-03-07 14:11:53]

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

[commit-bot: I haz the power, 2014-03-07 14:11:53]

Try jobs failed on following builders: linux_blink

[andersr, 2014-03-07 14:46:24]

The CQ bit was checked by andersr@opera.com

[commit-bot: I haz the power, 2014-03-07 14:47:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/185683003/60005

[commit-bot: I haz the power, 2014-03-07 15:49:31]

Change committed as 168743