Bug 104682 – [Skia] Graphics context uses stroke style from previous renderings

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Bruno Abinader (history only)]

Created attachment 178827 [details]
Layout test results (before)

While implementing a fix for bug 96408, I discovered that Skia wasn't properly resetting the graphics context, in special the stroke style/pattern, and thus using the one from last stroke style set. Please see attached the screenshots of fast/css3-text/css3-text-decoration/text-decoration-style.html layout test results before and after the fix proposed for this bug. You can notice the Ahem font character boundaries gets the text decoration stroke style when using "dashed" and "dotted" styles.

[Bruno Abinader (history only)]

Created attachment 178828 [details]
Layout test results (after)

[Bruno Abinader (history only)]

Created attachment 178830 [details]
Patch

Proposed patch.

[EFL EWS Bot]

Comment on attachment 178830 [details]
Patch

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

[Bruno Abinader (history only)]

Created attachment 178831 [details]
Patch

Proposed patch (2) - removes a variable that gets unused after changes.

[Bruno Abinader (history only)]

Note for reviewers: I am not certain whether make the graphics context save and then later restore its state for all platforms or just for Skia, as this _might_ affect overall performance (I have no reason to state that paintDecoration function can act as a bottleneck). Also, I am not a Skia specialist so there might be a clever solution for this. Thoughts?

[Bruno Abinader (history only)]

Just found out the same issue happens on Cairo/EFL port, so the "general" approach seems more proper (as seen on screenshots from bug 94110).

[Darin Adler]

Comment on attachment 178831 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:969
> +    // FIXME: This fixes a rendering issue in Skia - rect drawing uses stroke style from previous
> +    // rendering artifact.
> +    context->save();

Doesn’t make sense that this is Skia-specific. It’s the job of the GraphicsContext class to abstract things like this and make them work the same across multiple graphics systems. If it works on other platforms and is broken on Skia, it’s likely a bug in the GraphicsContext implementation, not something that should require changing this code.

Doing a save/restore every time we paint a decoration on an inline text box will likely be a measurable performance regression, so we only want to make that change if we absolutely have to. This bug needs further investigation.

[Darin Adler]

I understand that both Skia and Cairo show this bug, but we need to find out if the Mac platform using CG shows it, and if not, why not. There is probably a more efficient way to fix this than adding an unconditional save/restore.

[Bruno Abinader (history only)]

(In reply to comment #8)
> I understand that both Skia and Cairo show this bug, but we need to find out if the Mac platform using CG shows it, and if not, why not. There is probably a more efficient way to fix this than adding an unconditional save/restore.

Agreed, Darin, I understand this could reduce overall performance, however I am curious whether we go for a platform-specific approach or a platform-agnostic one, as the bug appears in more than one platform.

As for the Mac tests, I do not have a Mac of my own, so I'm afraid I am unable to provide such results, anyone volunteers? :)

[Darin Adler]

(In reply to comment #9)
> I am curious whether we go for a platform-specific approach or a platform-agnostic one, as the bug appears in more than one platform.

It’s the job of the GraphicsContext class to provide an interface that works the same across multiple platforms. If it’s not doing that successfully, we should fix that first before we make any changes to the client code using GraphicsContext.

[Bruno Abinader (history only)]

Skia-specific.

[Bruno Abinader (history only)]

FYI: This issue is now handled in http://crbug.com/229579

[Stephen Chenney]

It seems like a general error, not just Skia specific. I don't know why it works on Mac.

As with the other calls to updateGraphcisContext in InlineTextBox, the one for

if (textDecorations != TDNONE && paintInfo.phase != PaintPhaseSelection) {

should also use a GraphicsContextStateSaver. The paintDecorations code scribbles all over the state and does not restore it before returning.