Bug 174675 – Composition underline color is always black

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Manuel Rego Casasnovas]

Created attachment 315987 [details]
Test to reproduce the issue

Right now the composition underline (which appears when you for example enter Japanese characters) is always black, so if you have a black background you cannot see it.
It seems it could be interesting to use the color of the font to avoid this issue.

I'm attaching a test that will show the issue, I'll attach the current and expected results too.

[Manuel Rego Casasnovas]

Created attachment 315988 [details]
Current output

[Manuel Rego Casasnovas]

Created attachment 315989 [details]
Expected output

[Ryosuke Niwa]

Using the color of the font matches NSTextView so we should just do that.

[Manuel Rego Casasnovas]

Created attachment 316082 [details]
Patch

[Build Bot]

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


Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 155, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 119, in process_file
    self._files[abs_file_path] = self._files[abs_file_path] + kwargs['line_numbers']
TypeError: can only concatenate list (not "NoneType") to list


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

[Manuel Rego Casasnovas]

Created attachment 317402 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 317403 [details]
Patch

[Wenson Hsieh]

Comment on attachment 317403 [details]
Patch

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

> Source/WebCore/editing/CompositionUnderline.h:40
> +        , use_text_color(u)

WebKit style guidelines (https://webkit.org/code-style-guidelines/) recommend camelcase for members and local variables.

In general, instead of using bools like this to describe differences in behavior, we've been moving to enums and enum classes -- so this could be changed to something like:

    enum class CompositionUnderlineColor { Default, TextColor }

...so the purpose of the boolean is made explicit by its type.

[Manuel Rego Casasnovas]

Created attachment 317405 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the quick review!

(In reply to Wenson Hsieh from comment #8)
> Comment on attachment 317403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317403&action=review
> 
> > Source/WebCore/editing/CompositionUnderline.h:40
> > +        , use_text_color(u)
> 
> WebKit style guidelines (https://webkit.org/code-style-guidelines/)
> recommend camelcase for members and local variables.

Oops yeah, sorry about that (I was doing it before in Blink and they changed the style there lately
and I messed it up).

> In general, instead of using bools like this to describe differences in
> behavior, we've been moving to enums and enum classes -- so this could be
> changed to something like:
> 
>     enum class CompositionUnderlineColor { Default, TextColor }
> 
> ...so the purpose of the boolean is made explicit by its type.

True, uploaded new version using an enum. PTAL.

[Ryosuke Niwa]

Comment on attachment 317403 [details]
Patch

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

> Source/WebCore/editing/CompositionUnderline.h:37
> +    CompositionUnderline(unsigned s, unsigned e, bool u, const Color& c, bool t)

While we're at it, please rename s, e, u, c, t to more meaningful descriptive variable names.

> Source/WebCore/editing/CompositionUnderline.h:48
> +    bool use_text_color { true };

r- because this should be camelCase.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4268
> +            bool use_text_color = true;

r- because this should be camelCase.

> Source/WebKit/UIProcess/gtk/InputMethodFilter.cpp:156
> +        m_page->setComposition(m_preedit, Vector<CompositionUnderline> { CompositionUnderline(0, m_preedit.length(), true, Color(Color::black), false) },

Please use enum class as Wenson suggested.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6948
> +            bool use_text_color = true;

r- because this should be camelCase.

> LayoutTests/editing/composition-underline-color.html:2
> +<meta charset="utf-8">

Do we really need this meta element?

> LayoutTests/editing/composition-underline-color.html:20
> +document.getElementById("test").focus();
> +if (window.textInputController)
> +  textInputController.setMarkedText("foobar", 6, 0);

Instead of creating a pixel test, we can create a ref retest where we use two different font colors for text
hide the text with some overlapping div and expect the rendering to be different.
e.g. we can use ^^^ to make sure the text appears at the top, and then position an overlapping div with position: absolute.
That way, only composition underline would be visible.
Create two HTML files like that, and we should be able to make one of them -expected-mispatch.html

[Manuel Rego Casasnovas]

Created attachment 317419 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review. Uploaded new patch applying suggested changes.

(In reply to Ryosuke Niwa from comment #11)
> Comment on attachment 317403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317403&action=review
> 
> > Source/WebCore/editing/CompositionUnderline.h:37
> > +    CompositionUnderline(unsigned s, unsigned e, bool u, const Color& c, bool t)
> 
> While we're at it, please rename s, e, u, c, t to more meaningful
> descriptive variable names.

Done.

> > Source/WebCore/editing/CompositionUnderline.h:48
> > +    bool use_text_color { true };
> 
> r- because this should be camelCase.

Fixed.

> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4268
> > +            bool use_text_color = true;
> 
> r- because this should be camelCase.

Fixed.

> > Source/WebKit/UIProcess/gtk/InputMethodFilter.cpp:156
> > +        m_page->setComposition(m_preedit, Vector<CompositionUnderline> { CompositionUnderline(0, m_preedit.length(), true, Color(Color::black), false) },
> 
> Please use enum class as Wenson suggested.

Done.

> > Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6948
> > +            bool use_text_color = true;
> 
> r- because this should be camelCase.

Fixed.

> > LayoutTests/editing/composition-underline-color.html:2
> > +<meta charset="utf-8">
> 
> Do we really need this meta element?

Removed.

> > LayoutTests/editing/composition-underline-color.html:20
> > +document.getElementById("test").focus();
> > +if (window.textInputController)
> > +  textInputController.setMarkedText("foobar", 6, 0);
> 
> Instead of creating a pixel test, we can create a ref retest where we use
> two different font colors for text
> hide the text with some overlapping div and expect the rendering to be
> different.
> e.g. we can use ^^^ to make sure the text appears at the top, and then
> position an overlapping div with position: absolute.
> That way, only composition underline would be visible.
> Create two HTML files like that, and we should be able to make one of them
> -expected-mispatch.html

Really nice idea, I've replaced the pixed test by a -expected-mismatch one.

[Build Bot]

Comment on attachment 317419 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317422 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 317419 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317423 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 317419 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317424 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Created attachment 317426 [details]
Patch

[Build Bot]

Comment on attachment 317426 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317438 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 317426 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 317426 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317442 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Hm... your test appears to be blank on Mac. I wonder what happened there?

[Manuel Rego Casasnovas]

Created attachment 317558 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Ryosuke Niwa from comment #27)
> Hm... your test appears to be blank on Mac. I wonder what happened there?

I've found the problem with the test in Mac.
I was doing a blur() to avoid painting the caret, but that also removes the composition underline in Mac (it was not removing it in GTK+).

So, I've modified a little bit the test, now I don't do the blur() but I hide both the text "^^^^^" and the caret with 2 overlapping DIVs.
I've verified that it fails in current master and passes with this patch in Mac.

[Build Bot]

Comment on attachment 317558 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317582 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 317558 [details]
Patch

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

New failing tests:
editing/composition-underline-color.html

[Build Bot]

Created attachment 317593 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Now this is failing on WebKit 1.
The issue there seems to be that the composition underline is not an underline
but a background highlight which uses a color that is the same in the test and the expected-mismatch.
Would be fine to skip this test for WebKit 1?

[Ryosuke Niwa]

Comment on attachment 317558 [details]
Patch

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

> LayoutTests/ChangeLog:15
> +        * editing/composition-underline-color-expected-mismatch.html: Added.
> +        * editing/composition-underline-color.html: Added.

Please move this test into editing/input.

[Ryosuke Niwa]

Alexey, does DumpRenderTree has a special behavior for drawing IME underline?

[Manuel Rego Casasnovas]

Created attachment 317680 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review, but we still need to decide what to do with the test in WK1.

[Build Bot]

Comment on attachment 317680 [details]
Patch

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

New failing tests:
editing/input/composition-underline-color.html

[Build Bot]

Created attachment 317685 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 317680 [details]
Patch

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

New failing tests:
editing/input/composition-underline-color.html

[Build Bot]

Created attachment 317687 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alexey Proskuryakov]

> Alexey, does DumpRenderTree has a special behavior for drawing IME underline?

Nothing I can think of - it's drawn by WebCore anyway. But it might be that DumpRenderTree doesn't put the same attributes on the attributed string, or that WebHTMLView doesn't extract some of them. Does the patch make it work in MiniBrowser WK1 windows?

[Manuel Rego Casasnovas]

(In reply to Alexey Proskuryakov from comment #43)
> > Alexey, does DumpRenderTree has a special behavior for drawing IME underline?
> 
> Nothing I can think of - it's drawn by WebCore anyway. But it might be that
> DumpRenderTree doesn't put the same attributes on the attributed string, or
> that WebHTMLView doesn't extract some of them. Does the patch make it work
> in MiniBrowser WK1 windows?

I don't know how to input special characters so I can see the composition underline on Mac. And Linux ports don't have WK1.
I'd appreciate if someone can verify the behavior of this patch in WK1.
Thanks!

[Alexey Proskuryakov]

I found a walkthrough on the internet: <https://chinese.yabla.com/type-chinese-characters.php?platform=mac>.

[Ryosuke Niwa]

Hm... as far as I can tell, it works just fine in mini browser. But somehow the text color is used as the background color? in DumpRenderTree. I need to do a little more digging to figure out what's happening here.

[Ryosuke Niwa]

Created attachment 317853 [details]
Additional changes needed for Mac

The problem is that you're not taking care of the plain text case. See the newly attached patch you need to have.

[Manuel Rego Casasnovas]

Created attachment 317920 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Ryosuke Niwa from comment #47)
> Created attachment 317853 [details]
> Additional changes needed for Mac
> 
> The problem is that you're not taking care of the plain text case. See the
> newly attached patch you need to have.

Awesome! Thank you very much, indeed this solves the issues with the test in Mac WK1.
I've added your changes to my patch and upload it again, let's hope now everything is green.

[WebKit Commit Bot]

Comment on attachment 317920 [details]
Patch

Clearing flags on attachment: 317920

Committed r220639: <http://trac.webkit.org/changeset/220639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33871018>