Bug 37788 – selection background color incorrect with partially selected composition text

• Component: Text
• Status: NEW

[Hironori Bono]

(Copied from <http://crbug.com/40408>.)

1. Open the attached layout test with test_shell.

What is the expected output?

The word 'text' is rendered in red as shown in "setmarkedtext-color-webkit.png".

What do you see instead?

Our test_shell render the word 'text' twice. The first 'text' is rendered in red and the second 'text' 
is rendered in while as shown in "setmarkedtext-color-chromium.png".

Please use labels and text to provide additional information.

This issue is the root cause of WebKit Bug 31502 <https://bugs.webkit.org/show_bug.cgi?id=31502> on Windows Chrome. When renderer()->selectionForegroundColor() returns a valid color, WebKit uses this color to render IME selected text as shown in the following code snippet. (We cannot see IME selection on Windows Chrome just because the function returns white.) This issue happens only on Windows Chrome because this function doesn't return valid colors on Mac Chrome or Safari.

434:    if (haveSelection) {
435:        // Check foreground color first.
436:        Color foreground = paintInfo.forceBlackText ? Color::black : renderer()->selectionForegroundColor();
437:        if (foreground.isValid() && foreground != selectionFillColor) {

The easiest solution is adding the following code, which ignores the color returned from renderer()->selectionForegroundColor() when this selection is an IME selection. Nevertheless, I'm not sure if it is OK for WebKit people. Is it possible to give me your opinions?

+       if (useCustomUnderlines)
+           foreground = Color();

Regards,

Hironori Bono

[Dimitri Glazkov (Google)]

Can we have just a bit more context, like the name of the file where you're proposing the change?

[Hironori Bono]

Created attachment 53795 [details]
A possible fix (This is just for your information.)

Thank you for your comment.

(In reply to comment #1)
> Can we have just a bit more context, like the name of the file where you're
> proposing the change?

Sorry for the lack of any context about this bug. Even though InlineTextBox::paint() paints the background of a selection in the selection-background color unless the selection is created by an input method, it paints the foreground of a selection in the selection-foreground color even if the selection is created by an input method. So, when we set the selection-foreground color to white (like Windows Chrome), InlineTextBox::paint() renders the selection foreground created by an input method in white without painting its background in the selection-background color. Would it be possible to give me if this behavior is by design or a bug?

Regards,

Hironori Bono

[Ojan Vafai]

Bono-san, do you know why we use a different selection background color for marked text than we do for non-marked text? That seems like the underlying bug to me, but I'm not a regular IME user, so I'm not sure what the standard expectation is.

What do IE/Firefox do in these cases?

By the way, how are you testing this? The only way I could hit this situation is if I wrote some IME text and then had JS programmatically set the selection. Is there a way a user can hit this bug in the absence of JavaScript?

[Hironori Bono]

Created attachment 54252 [details]
A manual test case

Thank you for your comments.

(In reply to comment #3)

> By the way, how are you testing this? The only way I could hit this situation
> is if I wrote some IME text and then had JS programmatically set the selection.
> Is there a way a user can hit this bug in the absence of JavaScript?

Thank you for noticing this. Yes, we can reproduce this issue when we change the DOM selection while typing text on IME. I wrote a simple test case to reproduce this issue without using test_shell. (I should have posted this test case first. Sorry.)

What steps will reproduce the problem?
1. Open the attached test case on Windows Chrome:
2. Type text on IME, and;
3. Type a control key.

Expected Result:
We can see IME text

Actual Result:
We cannot see IME text except an underline.

Regards,

Hironori Bono

[Ojan Vafai]

CCing some people who've worked on this code before and removing the Chromium bit since it happens on Safari Windows as well.

Do you know why this bug only happens on Windows? On Mac, the marked text gets selected, but there's no visual indication of it.

I still don't understand why it's correct to not draw the background selection on marked text. It looks like that check was added back in due to bug 12594. But that bug doesn't explain why that behavior is correct.

TextEdit: Selecting during a composition shows no selection during the drag. But on mouseup, it confirms the composition, then shows a regular selection.
WordPad: Selecting during a composition confirms the composition on mousedown and then selection happens as normal.
WebKit nightly and Safari 4: Confirm the selection on mouse down, but don't select anything.

I don't see that there's a platform convention on Windows or Mac for selected, marked text, since no platform lets a user select it. I'm not a regular IME user though, so maybe there is some convention I'm missing.

The only way you can get selected IME text in WebKit is when the selection is set via JavaScript, right? It seems like we should render a normal selection background there. I wonder if we should confirm composition text when you set the selection as well.

[Hironori Bono]

Ojan,

Thank you for your comments. Probably my description for this issue was not so good. Sorry for my stupid description.

I would like to clarify one point before answering your questions: Windows Chrome DOES renders the IME selection text. Just we cannot see it because Windows Chrome renders it ALWAYS IN WHITE. (In fact, we can see it when we change the background color of the <input> element.)

(In reply to comment #5)
> CCing some people who've worked on this code before and removing the Chromium bit since it happens on Safari Windows as well.
> 
> Do you know why this bug only happens on Windows? On Mac, the marked text gets selected, but there's no visual indication of it.
>
> I don't see that there's a platform convention on Windows or Mac for selected, marked text, since no platform lets a user select it. I'm not a regular IME user though, so maybe there is some convention I'm missing.

You are right. there are not any platform convention for rendering the IME-selection text, and Windows Chrome DOES render the IME-selection text as I written above. That is, this issue is not the one whether or not to render an IME-selection text, but the one of picking up the color used for rendering an IME-selection text.

As you know, each platform uses its own color theme: RenderThemeMac for Mac Safari (and Chrome), RenderThemeChromiumWin for Windows Chrome, etc. When InlineTextBox::paint() renders the IME-selection forground, it calls RenderTheme::activeSelectionForegroundColor() via RenderObject::selectionForegroundColor(). This RenderTheme::activeSelectionForegroundColor() calls RenderThemeChromiumWin::platformActiveSelectionForegroundColor() on Windows. On the other hand, it returns an invalid color on Mac because RenderThemeMac::supportsSelectionForegroundColors() returns false.

> The only way you can get selected IME text in WebKit is when the selection is set via JavaScript, right?

As writen in Bug 31502, Editor::setComposition() also causes this issue. (Even though Chrome uses a workaround to avoid this issue, it causes Bug 31502.)

> It seems like we should render a normal selection background there. I wonder if we should confirm composition text when you set the selection as well.

Even though I'm wondering if I understand this comment correctly, I filed this issue because WebKit renders an IME-text background in the TEXT-BACKGROUND color even though it renders this IME text in the SELECTION-FOREGROUND color. (From the point of consistency, when a text background is rendered in a text-background color, its text foreground should be rendered in a text-foreground color. Even though this text is an IME text, I assume it is true, right?)

Finally, I don't have any intention to push this patch and it's up to you whether or not to fix this issue because I'm losing confidence of continuing my WebKit work.

Regards,

Hironori Bono

[Alexey Proskuryakov]

> I wonder if we should confirm composition text when you set the selection as well.

I don't think input methods are prepared to do a reasonable thing with selected unconfirmed text, so I agree, it sounds like we should.

[James Su]

(In reply to comment #3)
> Bono-san, do you know why we use a different selection background color for marked text than we do for non-marked text? That seems like the underlying bug to me, but I'm not a regular IME user, so I'm not sure what the standard expectation is.
> 
> What do IE/Firefox do in these cases?
> 
> By the way, how are you testing this? The only way I could hit this situation is if I wrote some IME text and then had JS programmatically set the selection. Is there a way a user can hit this bug in the absence of JavaScript?

An input method may change the selection when setting the composition (marked) text. Editor::setComposition() method support it. And you may refer to this document for details:
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/ApplicationKit/Protocols/NSTextInput_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSTextInput/setMarkedText:selectedRange:

However the selection range set by an input method should be completely inside the composition text.

[James Su]

*** Bug 40805 has been marked as a duplicate of this bug. ***

[James Su]

Please also refer to this duplicated bug report. I attached some screenshots there.

(In reply to comment #9)
> *** Bug 40805 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

(In reply to comment #10)
> Please also refer to this duplicated bug report. I attached some screenshots there.

Looking at those screenshots, it sounds like you're saying that the correct behavior would be to have selected/marked text to have the same background color as selected/non-marked text. Is that right?

[James Su]

(In reply to comment #11)
> (In reply to comment #10)
> > Please also refer to this duplicated bug report. I attached some screenshots there.
> 
> Looking at those screenshots, it sounds like you're saying that the correct behavior would be to have selected/marked text to have the same background color as selected/non-marked text. Is that right?

Actually, I don't care about whether these two selected background colors are same or not, as long as they are drawn correctly. IMHO, using the same color would be simpler and more consistent.

[James Su]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Please also refer to this duplicated bug report. I attached some screenshots there.
> > 
> > Looking at those screenshots, it sounds like you're saying that the correct behavior would be to have selected/marked text to have the same background color as selected/non-marked text. Is that right?
> 
> Actually, I don't care about whether these two selected background colors are same or not, as long as they are drawn correctly. IMHO, using the same color would be simpler and more consistent.

The current behavior is:
1. When the selection range covers the whole marked text, and there is no underline, then webkit draws the selection background correctly.
2. When the marked text has composition underline inside the selection range, then the selection background won't be drawn at all. That's exactly the behavior of current Safari.
3. When the marked text has no composition underline inside the selection range, and the selection range only covers part of the marked text, then webkit draws the selection foreground correctly but not the background, which causes completely wrong result.

[Ojan Vafai]

It makes sense to me that our selection background color should not depend on whether we are selecting marked text. Whether we should confirm the composition when setting the selection from JS is probably a different bug.

[James Su]

(In reply to comment #14)
> It makes sense to me that our selection background color should not depend on whether we are selecting marked text. Whether we should confirm the composition when setting the selection from JS is probably a different bug.

Agree. Shall we create a new bug for the JS thing? And what's webkit's current behavior?