Bug 15997 – SVG selection text foreground colour does not respect user settings

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Alp Toker]

When SVG text is selected, the foreground colour seems not to change, while it should use RenderObject::selectionForegroundColor().

This was noticed in the GTK+ port, where the text foreground selection colour chosen by the user is often light.

It looks like there is code that could be shared between SVGInlineTextBox and InlineTextBox to avoid having to fix the same issues twice.

[Eric Seidel (no email)]

Works for me on Mac.

[Gustavo Noronha (kov)]

Confirmed; I tried setting my theme to Clearlooks, and changed the text selection color to red. My browser's address bar selection works fine, as well as text inside a normal html page, but not on SVG.

[Eric Seidel (no email)]

We also seem to draw the selection with a gradient if the text is drawn with a gradient.

[MORITA Hajime]

Created attachment 45647 [details]
first attempt

[MORITA Hajime]

SVG didn't handle selection on glyph painting, so try to do in the patch.
Current SVG painting also has some problems about painting background and foreground,that prevent test to pass. The patch also fix the problem with
dividing paint into separete phases as HTML renderer does.

The patch re-baselined some testcases due to rendering result change.
I believe these changes are feasible. But not so sure...

[WebKit Review Bot]

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

[Eric Seidel (no email)]

I don't understand this patch.

[MORITA Hajime]

Thank you for your feedback.
The patch addresses multiple problems and seems too large... Sorry for that.
I'll file these problems as an another bug and tackle individually,
then come back to this bug.

first one is:
- https://bugs.webkit.org/show_bug.cgi?id=33069

[MORITA Hajime]

Comment on attachment 45647 [details]
first attempt

cancel review request at this time. I'll revise later.

[Nikolas Zimmermann]

Nice work! I understand most parts of the patch, and this looks like a very nice attempt. Though as Eric said it's quite heavy, and should be broken into smaller pieces. Especially the special SVG painting phases look weird to me, it should be possible to integrate with our existing CSS based painting phase concepts.

[MORITA Hajime]

Thank you for your insight!

> Especially the special SVG painting phases look weird to me, it
> should be possible to integrate with our existing CSS based painting phase
> concepts.

Although I agree this at a high-level viewpoint,
there are some complication in current implementation.

- Currently SVG has fill and stroke phases for text, which HTML doesn't have.
- During painting text, it is hard to change graphics context state,
  especially we cannot push/pop the state because of SVGPaintServerGradient
  which paint the rendering result with mask on its teardown().
- Because of above reason, we need more separate (sub-)phases than HTML.
  To avoid this, code tend to hacky as the last patch...

My current plan is to add more 2 sub-phases to paint selected glyph,
in addition to Ones would be introduced at Bug 33069.
But yes, it's weired...
I think we may need more lightweight graphics context switching mechanism
for this purpose.

[MORITA Hajime]

Created attachment 48304 [details]
patch v2

[WebKit Review Bot]

Attachment 48304 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/SVGInlineTextBox.cpp:405:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebCore/rendering/SVGInlineTextBox.cpp:440:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[MORITA Hajime]

Created attachment 48305 [details]
fix guideline violation

[MORITA Hajime]

Back from Bug 33069 and posted revised patch.
This patch introuces 2 more optional text painting subphases.

SVGTextChunkWalker now have too many callbacks and get complicated.
If the way this patch goes is OK, 
I would like to refactor to bring them into single callback to handle subphases.

[Nikolas Zimmermann]

Looks good to me, will give it a final review tomorrow, too tired at the moment.
I think we should have a chat about your further refactoring work, there are lots of important tasks todo to simplify SVG text layouting which is very complex atm, I'd like to hear your thoughts as well.

[Nikolas Zimmermann]

Comment on attachment 48305 [details]
fix guideline violation

Okay, patch looks fine - despite the fact that it adds more complexity.
But if you are willing to refactor this code anyways it's fine with me....

As you don't seem to be a committer, I'm going to take care of landing, will test locally first :-)
Are you available for an IRC discussion at some point?

[MORITA Hajime]

Thank you for reviewing and landing!
As we talked at IRC, I'll try successive refactorings after this one.

[Nikolas Zimmermann]

Okay, I have applied your patch.
Turns out there are 3 other pixel test changes, below the default threshold of 0.1%.
If you try "run-webkit-tests --tolerance 0 -p svg" you will see them. Marginal changes only, so I've just updated the pixel tests. For your new testcase results had to be updated, because DRT was changed just a dozen commits ago so I did that as well.

Patch is great - landing it now, congratulations on your first SVG patch :-)
Looking forward to the upcoming refactor patches...

[Nikolas Zimmermann]

Landed in r54556.