Bug 94110 – [cairo] Add support for text decoration "wavy" style

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Bruno Abinader (history only)]

The CSS3 property "text-decoration-style" accepts the following values: "solid | double | dotted | dashed | wavy" as seen on the link below:
http://dev.w3.org/csswg/css3-text/#text-decoration-style

All values except "wavy" were already supported on Cairo due to previous usage on "border-style" property:
http://www.w3.org/TR/css3-background/#the-border-style

This bug intends to provide Cairo platform support for "wavy" value (already parsed for "text-decoration-style" in bug 90958).

[KyungTae Kim]

Created attachment 168106 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168106 [details]
Patch

Attachment 168106 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14255370

[Martin Robinson]

Comment on attachment 168106 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:305
> +#if ENABLE(CSS3_TEXT_DECORATION)

Perhaps we can just always handle the WavyStroke style instead of conditionally on a feature that may or may not be related.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306
> +    if (style == WavyStroke) {

It seems that WavyStroke doesn't exist yet, so I guess this patch still depends on some future patch.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:308
> +        cairo_set_line_join(context, CAIRO_LINE_JOIN_BEVEL); // set line join to BEVEL (more suitable for 'wavy' than MITTER or ROUND)

You comment doesn't need to repeat in words what the code does, it just needs to explain why. This can just say:

// A bevelled line join is more suitable for wavy than miter or round.

Note that comments in WebKit should begin with a capital letter and end with a period.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:309
> +        const short step = strokeThickness; // up and down as strokeThickness (or left/right if line is vertical)

Why const short? strokeThickness is an int, so you might be truncating here.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:318
> +        if (point1OnPixelBoundaries.x() == point2OnPixelBoundaries.x()) {
> +            float y = point1OnPixelBoundaries.y() + step + 1;
> +            while (y <= point2OnPixelBoundaries.y() - 1) {
> +                signal = ~signal + 1; // alternates between +1 and -1
> +                cairo_line_to(context, point1OnPixelBoundaries.x() +  signal * step, y);
> +                y += step + 1;
> +            };
> +        } else {

There are a few things that could be improved here:

1. The code is a bit hard to follow and repeats the meat of the loop twice.
2. Instead of reusing isVerticalLine from above it does the check again.
3. It starts and stops one pixel from the starting and ending point and there's no indication why.
4. The code assumes that p2.(x,y) > p1.(x,y). I'm not sure if you can make this assumption here.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:320
> +            while (x <= point2OnPixelBoundaries.x() -1) {

The spacing is weird here at -1

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:326
> +        cairo_set_line_join(context, savedLineJoin); // restore line join

The comment is unnecessary here.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:331
>      if (patternWidth) {
>          // Do a rect fill of our endpoints.  This ensures we always have the
>          // appearance of being a border.  We then draw the actual dotted/dashed line.

Is there some reason this code does not apply for wavy strokes?

[KyungTae Kim]

Martin Robinson: Thank you for the review! That's very helpful!
Actually, this bug depends on the following bug for QT:
https://bugs.webkit.org/show_bug.cgi?id=93507

Why that is not listed on is, I couldn't find how to make a dependency. :)

I'll update the patch by your review soon.
Thank you.

[Bruno Abinader (history only)]

Comment on attachment 168106 [details]
Patch

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

Hi all,
Just sharing with you my informal review. I'm also looking forward to get patch from bug 93507 (Qt port) and the one I'm about to upload for Skia port, so we can finally have pixel results for text-decoration-style layout tests.

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:305
>> +#if ENABLE(CSS3_TEXT_DECORATION)
> 
> Perhaps we can just always handle the WavyStroke style instead of conditionally on a feature that may or may not be related.

This namespace has recently been renamed to CSS3_TEXT.

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:306
>> +    if (style == WavyStroke) {
> 
> It seems that WavyStroke doesn't exist yet, so I guess this patch still depends on some future patch.

The WavyStroke exists (see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/GraphicsContext.h#L143 for details), as it makes part of the CSS3 "text-decoration-style" property, however it's safeguarded by the CSS3_TEXT namespace, which is only enabled by default on EFL build for now.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:314
> +                signal = ~signal + 1; // alternates between +1 and -1

As suggested on bug 93507 as well, for readibility issues this could be changed to "signal = -signal;".

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:321
> +                signal = ~signal + 1; // alternates between +1 and -1

Ditto.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:328
> +#endif // CSS3_TEXT_DECORATION

s/CSS3_TEXT_DECORATION/CSS3_TEXT/

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:350
> +#if ENABLE(CSS3_TEXT_DECORATION)

Ditto.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:352
> +#endif // CSS3_TEXT_DECORATION

Ditto.

[KyungTae Kim]

Created attachment 171623 [details]
Patch

[Bruno Abinader (history only)]

(In reply to comment #6)
> Created an attachment (id=171623) [details]
> Patch

The patch looks great and works fine on my EFL layout test run. As we discussed through IRC, we could later merge this function together with drawErrorUnderline on a separate patch. There is also a possibility to use pixmaps (as Chromium does and Qt does internally on QPainter) instead of generating the wavy lines, these were commented by Simon on bug 93507.

[KyungTae Kim]

Created attachment 174059 [details]
Patch

[Martin Robinson]

Looking at the results of this output they seem similar to what DrawErrorUnderline.h produces. Bruno mentioned on the mailing list the possibility of re-using that code for all platforms. What was the result of that investigation?

[Lamarque V. Souza]

(In reply to comment #9)
> Looking at the results of this output they seem similar to what DrawErrorUnderline.h produces. Bruno mentioned on the mailing list the possibility of re-using that code for all platforms. What was the result of that investigation?

drawErrorUnderline does not draw vertical lines and is more complex than this implementation, so Bruno and me thought in doing the opposite: remove drawErrorUnderline and use drawLine with wavyStroke instead. That is not platform agnostic but helps removing duplicated code. There is an implementation for the Qt platform in https://bugs.webkit.org/show_bug.cgi?id=93507#c27

[Martin Robinson]

Okay. That seems like a reasonable approach.

[KyungTae Kim]

I think committing this patch first, and then removing duplicate code on another bug seems better.
Martin Robinson, could you please review?

[KyungTae Kim]

Created attachment 175068 [details]
mozilla's implementation (zoomlevel 1.0)

[KyungTae Kim]

Created attachment 175069 [details]
mozilla's implementation (larger zoomlevel)

[KyungTae Kim]

Created attachment 175084 [details]
this implementation (zoom 1.0)

The Mozilla implementation has a wave with a larger amplitude and a small frequency.
The amplitude or frequency of wavy line is not defined on a spec.
If this implementation should be modified like Mozilla, it's same issue with other ports.
Bruno Abinader and Lamarque V. Souza, how do you think about that?

[Lamarque V. Souza]

(In reply to comment #15)
> Created an attachment (id=175084) [details]
> this implementation (zoom 1.0)
> 
> The Mozilla implementation has a wave with a larger amplitude and a small frequency.
> The amplitude or frequency of wavy line is not defined on a spec.
> If this implementation should be modified like Mozilla, it's same issue with other ports.
> Bruno Abinader and Lamarque V. Souza, how do you think about that?

I think we should follow Mozilla's implementation. As an browser user I would like to see pages looking the same in any browser.

[KyungTae Kim]

Created attachment 177092 [details]
Patch

[KyungTae Kim]

Created attachment 177155 [details]
screenshot (zoom 1.0, 1.5, 2.0, 3.0)

screenshot from above patch  with zoom level 1.0, 1.5, 2.0, 3.0.

[Bruno Abinader (history only)]

Hi KyungTae, sorry for not replying earlier (got back to work today), yes, the spec gives us space for interpretation on this aspect, and following the Mozilla implementation sounds a good idea for me as well. From the screenshots, it looks very stylish :) Good work!

[Lamarque V. Souza]

(In reply to comment #17)
> Created an attachment (id=177092) [details]
> Patch

I am porting your changes to the Qt patch [1] and have some small doubts. Why the vertical implementation contains "float y = y1 + step + 1;" and the horizontal contains "float x = x1 + step;". I guess the vertical one does not need the "+ 1", right?

Another question regards the condition in the while loops, for the horizontal one it is "(x <= x2 - 1)", why the "-1" here? In Qt we subtract one pixel from the coordinates because Qt and WebKit threat coordinates slightly different. Does the same happen in Cairo?

[1] https://bugs.webkit.org/show_bug.cgi?id=93507

[Bruno Abinader (history only)]

Fixed in bug 92868. Closing bug.

[Zan Dobersek]

Comment on attachment 177092 [details]
Patch

Clearing the review flag on the last patch.