Bug 122716 – Broken text rendering when input field has selection.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

In the last builds, I see broken text rendering in some places. It seems the text color is wrong.

Created attachment 214086 [details]
Patch

[Antti Koivisto]

Comment on attachment 214086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214086&action=review

This needs a test case. It should be easy to make one as a ref test. See tests with -expected.html under LayoutTests/fast/text/ for example.

r- because lack of test case or even information how to reproduce the bug.

> Source/WebCore/ChangeLog:3
> +        Broken text rendering in some cases.

What cases? The bug description doesn't say either.

> Source/WebCore/rendering/InlineTextBox.cpp:551
> -    TextPaintStyle selectionPaintStyle = computeTextSelectionPaintStyle(textPaintStyle, renderer(), lineStyle, paintInfo, paintSelectedTextOnly, paintSelectedTextSeparately, selectionShadow);
> -
> +    TextPaintStyle selectionPaintStyle = computeTextSelectionPaintStyle(textPaintStyle, renderer(), lineStyle, paintInfo, haveSelection, paintSelectedTextOnly, paintSelectedTextSeparately, selectionShadow);

Instead of adding an extra boolean parameter it would be better to just not call computeTextSelectionPaintStyle if haveSelection is false.

(In reply to comment #2)
> (From update of attachment 214086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214086&action=review
> 
> This needs a test case. It should be easy to make one as a ref test. See tests with -expected.html under LayoutTests/fast/text/ for example.
> 
> r- because lack of test case or even information how to reproduce the bug.
> 
> > Source/WebCore/ChangeLog:3
> > +        Broken text rendering in some cases.
> 
> What cases? The bug description doesn't say either.
> 

Sorry for the short description :) I added an url where it can be reproduced, at least when running WinCairo. 

> > Source/WebCore/rendering/InlineTextBox.cpp:551
> > -    TextPaintStyle selectionPaintStyle = computeTextSelectionPaintStyle(textPaintStyle, renderer(), lineStyle, paintInfo, paintSelectedTextOnly, paintSelectedTextSeparately, selectionShadow);
> > -
> > +    TextPaintStyle selectionPaintStyle = computeTextSelectionPaintStyle(textPaintStyle, renderer(), lineStyle, paintInfo, haveSelection, paintSelectedTextOnly, paintSelectedTextSeparately, selectionShadow);
> 
> Instead of adding an extra boolean parameter it would be better to just not call computeTextSelectionPaintStyle if haveSelection is false.

OK, but isn't there some initialization at the beginning of computeTextSelectionPaintStyle that needs to be done even if haveSelection is false?

[Antti Koivisto]

(In reply to comment #3)
> OK, but isn't there some initialization at the beginning of computeTextSelectionPaintStyle that needs to be done even if haveSelection is false?

Maybe though it seems wrong that we actually use any of this stuff when haveSelection is false.

[Antti Koivisto]

I don't see anything obviously wrong with the page on mac. Can you include a screenshot?

It is possible that this is just exposing a WinCairo bug.

(In reply to comment #5)
> I don't see anything obviously wrong with the page on mac. Can you include a screenshot?
> 

Yes, will do. I also see it when searching for changesets: e.g: http://trac.webkit.org/search?q=157000

> It is possible that this is just exposing a WinCairo bug.

That could very well be. I also noticed that there used to be a check for haveSelection.

Created attachment 214100 [details]
Image showing broken text rendering on WinCairo

[Antti Koivisto]

Mac rendering of that page looks fine.

(In reply to comment #8)
> Mac rendering of that page looks fine.

Ok, will have a look at platform code :)

[Antti Koivisto]

Does this fix it?

--- Source/WebCore/rendering/InlineTextBox.cpp	(revision 157368)
+++ Source/WebCore/rendering/InlineTextBox.cpp	(working copy)
@@ -610,7 +610,7 @@
 
     int sPos = 0;
     int ePos = 0;
-    if (paintSelectedTextOnly || paintSelectedTextSeparately)
+    if (haveSelection && (paintSelectedTextOnly || paintSelectedTextSeparately))
         selectionStartEnd(sPos, ePos);
 
     if (m_truncation != cNoTruncation) {

(In reply to comment #10)
> Does this fix it?
> 
> --- Source/WebCore/rendering/InlineTextBox.cpp    (revision 157368)
> +++ Source/WebCore/rendering/InlineTextBox.cpp    (working copy)
> @@ -610,7 +610,7 @@
> 
>      int sPos = 0;
>      int ePos = 0;
> -    if (paintSelectedTextOnly || paintSelectedTextSeparately)
> +    if (haveSelection && (paintSelectedTextOnly || paintSelectedTextSeparately))
>          selectionStartEnd(sPos, ePos);
> 
>      if (m_truncation != cNoTruncation) {

Yes, that does the trick :) Commit?

[Antti Koivisto]

If you could make a reduced test case that would be neat. Maybe I can figure out how to make the bug visible on mac too.

Created attachment 214105 [details]
Testcase for broken text rendering

(In reply to comment #12)
> If you could make a reduced test case that would be neat. Maybe I can figure out how to make the bug visible on mac too.

I attached a somewhat reduced test case. Pretty weird though, seems there needs to be an input field with some content, which receives focus, for the bug to appear ...

Created attachment 214133 [details]
Patch

(In reply to comment #10)
> Does this fix it?
> 
> --- Source/WebCore/rendering/InlineTextBox.cpp    (revision 157368)
> +++ Source/WebCore/rendering/InlineTextBox.cpp    (working copy)
> @@ -610,7 +610,7 @@
> 
>      int sPos = 0;
>      int ePos = 0;
> -    if (paintSelectedTextOnly || paintSelectedTextSeparately)
> +    if (haveSelection && (paintSelectedTextOnly || paintSelectedTextSeparately))
>          selectionStartEnd(sPos, ePos);
> 
>      if (m_truncation != cNoTruncation) {

Updated the patch with your fix, and a testcase.

[Antti Koivisto]

Comment on attachment 214133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214133&action=review

The test case is missing a file. It also doesn't work on mac (it might by setting some selection colours) but that's ok. At least it can be tested exercises the code paths (and can be tested manually on wincairo).

> LayoutTests/ChangeLog:8
> +        * fast/text/text-rendering-with-input-selection-expected.html: Added.

Ref test should have two files: text-rendering-with-input-selection.html and  text-rendering-with-input-selection-expected.html

The test works by generating bitmap from both and comparing them. -expected should contain something that renders the same exact way as the test case but does so without hitting the bug.

Created attachment 214137 [details]
Patch

(In reply to comment #17)
> (From update of attachment 214133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214133&action=review
> 
> The test case is missing a file. It also doesn't work on mac (it might by setting some selection colours) but that's ok. At least it can be tested exercises the code paths (and can be tested manually on wincairo).
> 
> > LayoutTests/ChangeLog:8
> > +        * fast/text/text-rendering-with-input-selection-expected.html: Added.
> 
> Ref test should have two files: text-rendering-with-input-selection.html and  text-rendering-with-input-selection-expected.html
> 
> The test works by generating bitmap from both and comparing them. -expected should contain something that renders the same exact way as the test case but does so without hitting the bug.

Thanks for clearifying, updated patch :)

[Antti Koivisto]

Comment on attachment 214137 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 214137 [details]
Patch

Clearing flags on attachment: 214137

Committed r157392: <http://trac.webkit.org/changeset/157392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #20)
> (From update of attachment 214137 [details])
> r=me

Thanks :)

On another note, I was just wondering if the function updateGraphicsContext when called with UseEmphasisMarkColor should set the fill color from the emphasisMarkColor, instead of the stroke color? I'm not sure though ...

[Antti Koivisto]

> On another note, I was just wondering if the function updateGraphicsContext when called with UseEmphasisMarkColor should set the fill color from the emphasisMarkColor, instead of the stroke color? I'm not sure though ...

You are right, looks like that has been switched around. Can you file a bug? Test case would be nice too :)

(In reply to comment #24)
> > On another note, I was just wondering if the function updateGraphicsContext when called with UseEmphasisMarkColor should set the fill color from the emphasisMarkColor, instead of the stroke color? I'm not sure though ...
> 
> You are right, looks like that has been switched around. Can you file a bug? Test case would be nice too :)

Yes, will file a bug, and see if I am able to come up with a testcase aswell ;)