Bug 181485 – REGRESSION(r222507): Composition highlight doesn't render when using IME

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/35896516>

[Wenson Hsieh]

Created attachment 330934 [details]
Fixes the bug

[Ryosuke Niwa]

Comment on attachment 330934 [details]
Fixes the bug

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

> Source/WebCore/rendering/InlineTextBox.cpp:1047
> +    paintTextSubrangeBackground(context, boxOrigin, Color::compositionFill, clampedOffset(renderer().frame().editor().compositionStart()), clampedOffset(renderer().frame().editor().compositionEnd()));

We should make Color(RGBA32 color, bool valid = true) explicit since RGBA32 is just unsigned so that mistakes like this will be harder to make
(not suggesting that we should do it in this patch or that you should do it).

[Wenson Hsieh]

Comment on attachment 330934 [details]
Fixes the bug

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

Thanks for the review!

> Source/WebCore/ChangeLog:11
> +        Test: editing/composition-highlight.html

Whoops, test here should be "editing/marked-text-appearance.html", not "editing/composition-highlight.html". I'll fix this before landing.

>> Source/WebCore/rendering/InlineTextBox.cpp:1047
>> +    paintTextSubrangeBackground(context, boxOrigin, Color::compositionFill, clampedOffset(renderer().frame().editor().compositionStart()), clampedOffset(renderer().frame().editor().compositionEnd()));
> 
> We should make Color(RGBA32 color, bool valid = true) explicit since RGBA32 is just unsigned so that mistakes like this will be harder to make
> (not suggesting that we should do it in this patch or that you should do it).

Definitely a good idea. I think Dan Bates had ideas on how to make this more robust, too...I'll file a followup for this.

[EWS Watchlist]

Comment on attachment 330934 [details]
Fixes the bug

Attachment 330934 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6025473

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 330971 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330934 [details]
Fixes the bug

Attachment 330934 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6026655

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 330987 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Created attachment 330994 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 330994 [details]
Patch for landing

Clearing flags on attachment: 330994

Committed r226753: <https://trac.webkit.org/changeset/226753>

[Darin Adler]

Comment on attachment 330934 [details]
Fixes the bug

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:1047
>>> +    paintTextSubrangeBackground(context, boxOrigin, Color::compositionFill, clampedOffset(renderer().frame().editor().compositionStart()), clampedOffset(renderer().frame().editor().compositionEnd()));
>> 
>> We should make Color(RGBA32 color, bool valid = true) explicit since RGBA32 is just unsigned so that mistakes like this will be harder to make
>> (not suggesting that we should do it in this patch or that you should do it).
> 
> Definitely a good idea. I think Dan Bates had ideas on how to make this more robust, too...I'll file a followup for this.

I have a solution to this ambiguity coming in my color patch; the total patch is in bug 180620 but I will be breaking off smaller pieces of it and landing them.