Bug 174675

Summary: Composition underline color is always black
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: HTML EditingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, commit-queue, dbates, jfernandez, rego, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=554893
Attachments:
Description Flags
Test to reproduce the issue
none
Current output
none
Expected output
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Additional changes needed for Mac
none
Patch none

Description Manuel Rego Casasnovas 2017-07-20 07:42:17 PDT
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.
Comment 1 Manuel Rego Casasnovas 2017-07-20 07:42:32 PDT
Created attachment 315988 [details]
Current output
Comment 2 Manuel Rego Casasnovas 2017-07-20 07:42:54 PDT
Created attachment 315989 [details]
Expected output
Comment 3 Ryosuke Niwa 2017-07-20 13:59:35 PDT
Using the color of the font matches NSTextView so we should just do that.
Comment 4 Manuel Rego Casasnovas 2017-07-21 03:56:13 PDT
Created attachment 316082 [details]
Patch
Comment 5 Build Bot 2017-07-21 03:58:46 PDT
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.
Comment 6 Manuel Rego Casasnovas 2017-08-06 23:18:10 PDT
Created attachment 317402 [details]
Patch
Comment 7 Manuel Rego Casasnovas 2017-08-06 23:42:54 PDT
Created attachment 317403 [details]
Patch
Comment 8 Wenson Hsieh 2017-08-06 23:50:59 PDT
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.
Comment 9 Manuel Rego Casasnovas 2017-08-07 00:09:26 PDT
Created attachment 317405 [details]
Patch
Comment 10 Manuel Rego Casasnovas 2017-08-07 00:10:44 PDT
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.
Comment 11 Ryosuke Niwa 2017-08-07 00:10:50 PDT
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
Comment 12 Manuel Rego Casasnovas 2017-08-07 08:03:02 PDT
Created attachment 317419 [details]
Patch
Comment 13 Manuel Rego Casasnovas 2017-08-07 08:07:13 PDT
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.
Comment 14 Build Bot 2017-08-07 09:12:40 PDT
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
Comment 15 Build Bot 2017-08-07 09:12:41 PDT
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
Comment 16 Build Bot 2017-08-07 09:18:01 PDT
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
Comment 17 Build Bot 2017-08-07 09:18:02 PDT
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
Comment 18 Build Bot 2017-08-07 09:29:03 PDT
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
Comment 19 Build Bot 2017-08-07 09:29:04 PDT
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
Comment 20 Manuel Rego Casasnovas 2017-08-07 09:44:56 PDT
Created attachment 317426 [details]
Patch
Comment 21 Build Bot 2017-08-07 10:52:23 PDT
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
Comment 22 Build Bot 2017-08-07 10:52:24 PDT
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
Comment 23 Build Bot 2017-08-07 10:59:42 PDT
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
Comment 24 Build Bot 2017-08-07 10:59:44 PDT
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
Comment 25 Build Bot 2017-08-07 11:19:06 PDT
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
Comment 26 Build Bot 2017-08-07 11:19:08 PDT
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
Comment 27 Ryosuke Niwa 2017-08-07 17:07:04 PDT
Hm... your test appears to be blank on Mac. I wonder what happened there?
Comment 28 Manuel Rego Casasnovas 2017-08-08 03:40:45 PDT
Created attachment 317558 [details]
Patch
Comment 29 Manuel Rego Casasnovas 2017-08-08 03:42:27 PDT
(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.
Comment 30 Build Bot 2017-08-08 10:15:03 PDT
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
Comment 31 Build Bot 2017-08-08 10:15:05 PDT
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
Comment 32 Build Bot 2017-08-08 12:04:49 PDT
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
Comment 33 Build Bot 2017-08-08 12:04:50 PDT
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
Comment 34 Manuel Rego Casasnovas 2017-08-08 15:44:57 PDT
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?
Comment 35 Ryosuke Niwa 2017-08-08 16:07:26 PDT
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.
Comment 36 Ryosuke Niwa 2017-08-08 16:08:10 PDT
Alexey, does DumpRenderTree has a special behavior for drawing IME underline?
Comment 37 Manuel Rego Casasnovas 2017-08-08 23:11:51 PDT
Created attachment 317680 [details]
Patch
Comment 38 Manuel Rego Casasnovas 2017-08-08 23:28:39 PDT
Thanks for the review, but we still need to decide what to do with the test in WK1.
Comment 39 Build Bot 2017-08-09 00:21:53 PDT
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
Comment 40 Build Bot 2017-08-09 00:21:54 PDT
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
Comment 41 Build Bot 2017-08-09 00:34:04 PDT
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
Comment 42 Build Bot 2017-08-09 00:34:06 PDT
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
Comment 43 Alexey Proskuryakov 2017-08-09 11:05:11 PDT
> 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?
Comment 44 Manuel Rego Casasnovas 2017-08-09 14:24:38 PDT
(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!
Comment 45 Alexey Proskuryakov 2017-08-09 15:17:40 PDT
I found a walkthrough on the internet: <https://chinese.yabla.com/type-chinese-characters.php?platform=mac>.
Comment 46 Ryosuke Niwa 2017-08-09 21:42:54 PDT
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.
Comment 47 Ryosuke Niwa 2017-08-10 14:48:15 PDT
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.
Comment 48 Manuel Rego Casasnovas 2017-08-11 00:47:09 PDT
Created attachment 317920 [details]
Patch
Comment 49 Manuel Rego Casasnovas 2017-08-11 01:01:38 PDT
(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.
Comment 50 WebKit Commit Bot 2017-08-13 23:31:38 PDT
Comment on attachment 317920 [details]
Patch

Clearing flags on attachment: 317920

Committed r220639: <http://trac.webkit.org/changeset/220639>
Comment 51 WebKit Commit Bot 2017-08-13 23:31:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Radar WebKit Bug Importer 2017-08-13 23:32:51 PDT
<rdar://problem/33871018>