Bug 104682 - [Skia] Graphics context uses stroke style from previous renderings
Summary: [Skia] Graphics context uses stroke style from previous renderings
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bruno Abinader (history only)
URL:
Keywords:
Depends on: 96408
Blocks: 100546 93509
  Show dependency treegraph
 
Reported: 2012-12-11 10:11 PST by Bruno Abinader (history only)
Modified: 2013-05-10 15:30 PDT (History)
7 users (show)

See Also:


Attachments
Layout test results (before) (7.07 KB, image/png)
2012-12-11 10:11 PST, Bruno Abinader (history only)
no flags Details
Layout test results (after) (6.26 KB, image/png)
2012-12-11 10:12 PST, Bruno Abinader (history only)
no flags Details
Patch (3.14 KB, patch)
2012-12-11 10:18 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2012-12-11 10:27 PST, Bruno Abinader (history only)
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Abinader (history only) 2012-12-11 10:11:44 PST
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.
Comment 1 Bruno Abinader (history only) 2012-12-11 10:12:19 PST
Created attachment 178828 [details]
Layout test results (after)
Comment 2 Bruno Abinader (history only) 2012-12-11 10:18:27 PST
Created attachment 178830 [details]
Patch

Proposed patch.
Comment 3 EFL EWS Bot 2012-12-11 10:24:16 PST
Comment on attachment 178830 [details]
Patch

Attachment 178830 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15257744
Comment 4 Bruno Abinader (history only) 2012-12-11 10:27:42 PST
Created attachment 178831 [details]
Patch

Proposed patch (2) - removes a variable that gets unused after changes.
Comment 5 Bruno Abinader (history only) 2012-12-11 10:32:37 PST
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?
Comment 6 Bruno Abinader (history only) 2012-12-17 08:02:50 PST
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 7 Darin Adler 2013-01-15 15:46:26 PST
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.
Comment 8 Darin Adler 2013-01-15 15:47:21 PST
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.
Comment 9 Bruno Abinader (history only) 2013-01-16 05:05:07 PST
(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? :)
Comment 10 Darin Adler 2013-01-16 09:45:16 PST
(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.
Comment 11 Bruno Abinader (history only) 2013-05-08 05:57:16 PDT
Skia-specific.
Comment 12 Bruno Abinader (history only) 2013-05-08 06:10:59 PDT
FYI: This issue is now handled in http://crbug.com/229579
Comment 13 Stephen Chenney 2013-05-10 15:30:24 PDT
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.