Bug 33069 – svg background color on selected text goes wrong when text has gradient stroke.

• Component: SVG
• Status: RESOLVED FIXED

[MORITA Hajime]

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: 
52638

How to reproduce:
open attached XHTML file.

Expected result:
All test has blue background.

Actual Result:
second line of text has gradient background.

[MORITA Hajime]

Created attachment 45697 [details]
first attempt

[WebKit Review Bot]

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

[MORITA Hajime]

Created attachment 45739 [details]
fix guideline violation

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45739 [details] without any errors.

[Eric Seidel (no email)]

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?

[MORITA Hajime]

Thank you for quick feedback!

(In reply to comment #5)
> Do they all use gradient fill text?
No. 
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.

[Nikolas Zimmermann]

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 :-)

[MORITA Hajime]

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.

[Eric Seidel (no email)]

Comment on attachment 45739 [details]
fix guideline violation

We'll see if the commit-queue can stomach this monster patch.

[WebKit Commit Bot]

Comment on attachment 45739 [details]
fix guideline violation

Clearing flags on attachment: 45739

Committed r52904: <http://trac.webkit.org/changeset/52904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.