Bug 15997 - SVG selection text foreground colour does not respect user settings
Summary: SVG selection text foreground colour does not respect user settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Cairo, Gtk
Depends on:
Blocks:
 
Reported: 2007-11-15 03:18 PST by Alp Toker
Modified: 2010-02-09 09:02 PST (History)
6 users (show)

See Also:


Attachments
first attempt (1.26 MB, patch)
2009-12-30 01:19 PST, MORITA Hajime
no flags Details | Formatted Diff | Diff
patch v2 (183.66 KB, patch)
2010-02-07 02:57 PST, MORITA Hajime
no flags Details | Formatted Diff | Diff
fix guideline violation (183.64 KB, patch)
2010-02-07 03:14 PST, MORITA Hajime
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2007-11-15 03:18:29 PST
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.
Comment 1 Eric Seidel (no email) 2007-12-15 12:22:23 PST
Works for me on Mac.
Comment 2 Gustavo Noronha (kov) 2009-03-16 09:32:09 PDT
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.
Comment 3 Eric Seidel (no email) 2009-05-03 15:43:40 PDT
We also seem to draw the selection with a gradient if the text is drawn with a gradient.
Comment 4 MORITA Hajime 2009-12-30 01:19:33 PST
Created attachment 45647 [details]
first attempt
Comment 5 MORITA Hajime 2009-12-30 01:24:38 PST
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...
Comment 6 WebKit Review Bot 2009-12-30 01:27:14 PST
style-queue ran check-webkit-style on attachment 45647 [details] without any errors.
Comment 7 Eric Seidel (no email) 2009-12-30 09:44:05 PST
I don't understand this patch.
Comment 8 MORITA Hajime 2009-12-30 19:17:51 PST
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
Comment 9 MORITA Hajime 2009-12-30 20:55:22 PST
Comment on attachment 45647 [details]
first attempt

cancel review request at this time. I'll revise later.
Comment 10 Nikolas Zimmermann 2010-01-02 13:43:52 PST
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.
Comment 11 MORITA Hajime 2010-01-02 19:15:54 PST
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.
Comment 12 MORITA Hajime 2010-02-07 02:57:07 PST
Created attachment 48304 [details]
patch v2
Comment 13 WebKit Review Bot 2010-02-07 03:02:29 PST
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.
Comment 14 MORITA Hajime 2010-02-07 03:14:36 PST
Created attachment 48305 [details]
fix guideline violation
Comment 15 MORITA Hajime 2010-02-07 03:23:07 PST
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.
Comment 16 Nikolas Zimmermann 2010-02-07 18:38:19 PST
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.
Comment 17 Nikolas Zimmermann 2010-02-08 12:52:39 PST
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?
Comment 18 MORITA Hajime 2010-02-08 16:38:11 PST
Thank you for reviewing and landing!
As we talked at IRC, I'll try successive refactorings after this one.
Comment 19 Nikolas Zimmermann 2010-02-09 08:47:28 PST
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...
Comment 20 Nikolas Zimmermann 2010-02-09 09:02:59 PST
Landed in r54556.