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.
Created attachment 178828 [details] Layout test results (after)
Created attachment 178830 [details] Patch Proposed patch.
Comment on attachment 178830 [details] Patch Attachment 178830 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15257744
Created attachment 178831 [details] Patch Proposed patch (2) - removes a variable that gets unused after changes.
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?
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).
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.
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.
(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? :)
(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.
Skia-specific.
FYI: This issue is now handled in http://crbug.com/229579
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.