Bug 33069

Summary: svg background color on selected text goes wrong when text has gradient stroke.
Product: WebKit Reporter: MORITA Hajime <morrita>
Component: SVGAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, hamaji, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Description Flags
xhtml to reproduce
first attempt
fix guideline violation none

Description MORITA Hajime 2009-12-30 19:13:32 PST
Created attachment 45695 [details]
xhtml to reproduce

When text has gradient stroke e.g. "stroke: url(#gradient); stroke-width: 1pt;", and
these text are selected, its background unexpectedly painted with the gradient.

svn revision: 

How to reproduce:
open attached XHTML file.

Expected result:
All test has blue background.

Actual Result:
second line of text has gradient background.
Comment 1 MORITA Hajime 2009-12-30 21:31:41 PST
Created attachment 45697 [details]
first attempt
Comment 2 WebKit Review Bot 2009-12-30 21:32:41 PST
Attachment 45697 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/SVGInlineTextBox.cpp:379:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1
Comment 3 MORITA Hajime 2010-01-02 07:59:57 PST
Created attachment 45739 [details]
fix guideline violation
Comment 4 WebKit Review Bot 2010-01-02 08:05:08 PST
style-queue ran check-webkit-style on attachment 45739 [details] without any errors.
Comment 5 Eric Seidel (no email) 2010-01-02 12:15:35 PST
Comment on attachment 45739 [details]
fix guideline violation

In general this seems like a good idea to me.  I would like to know more about why the various layout tests changed.  It's not clear to me from the diffs why each layout test changed.  Do they all use gradient fill text?
Comment 6 MORITA Hajime 2010-01-02 18:23:00 PST
Thank you for quick feedback!

(In reply to comment #5)
> Do they all use gradient fill text?
This change affects SVG text selection rendering
even if it is not with gradient. 

Original code paints background twice; 
Once at fill subphase and another at stroke subphase,
in following order:

1. paintCharacters() for fill
1.1. paint background
1.2. paint glyph for fill
2. paintCharacters() for stroke
2.1. paint background (again)
2.2. paint glyph for stroke.

So 2.1 (background painting on stroke subphase) overwrites
the filled glyphs (with half opacity).
The patch split background painting out to its own subphase,
then filled glyphs are no longer overwritten.
This change requires some test results rebasing.
But it seems reasonable for me.

Slightly OT:
Painting foreground and text decorations may have same issue.
But currently I have no idea to unveil them.
Comment 7 Nikolas Zimmermann 2010-01-06 18:57:00 PST
Comment on attachment 45739 [details]
fix guideline violation

Looks good to me, I've checked all results and they're just fine now. Glad you've started this work - do you plan to work more on SVG Text?
We have some important TODO items in that area - I'd be glad if you're interessted :-)
Comment 8 MORITA Hajime 2010-01-06 22:06:57 PST
Thank you for reviewing!

(In reply to comment #7)
> (From update of attachment 45739 [details])
> We have some important TODO items in that area - I'd be glad if you're
> interessted :-)
Well, I have certain interest and will go back to Bug 15997 next.
And any further suggestions and ideas to go are appreciated.
Comment 9 Eric Seidel (no email) 2010-01-06 23:45:29 PST
Comment on attachment 45739 [details]
fix guideline violation

We'll see if the commit-queue can stomach this monster patch.
Comment 10 WebKit Commit Bot 2010-01-07 00:07:32 PST
Comment on attachment 45739 [details]
fix guideline violation

Clearing flags on attachment: 45739

Committed r52904: <http://trac.webkit.org/changeset/52904>
Comment 11 WebKit Commit Bot 2010-01-07 00:07:41 PST
All reviewed patches have been landed.  Closing bug.