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.
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 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.
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 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
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.
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
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
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
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
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
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
(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.
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
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
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?
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
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, 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?
(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!
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.
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.
(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.
2017-07-20 07:42 PDT, Manuel Rego Casasnovas
2017-07-20 07:42 PDT, Manuel Rego Casasnovas
2017-07-20 07:42 PDT, Manuel Rego Casasnovas
2017-07-21 03:56 PDT, Manuel Rego Casasnovas
2017-08-06 23:18 PDT, Manuel Rego Casasnovas
2017-08-06 23:42 PDT, Manuel Rego Casasnovas
2017-08-07 00:09 PDT, Manuel Rego Casasnovas
2017-08-07 08:03 PDT, Manuel Rego Casasnovas
2017-08-07 09:12 PDT, Build Bot
2017-08-07 09:18 PDT, Build Bot
2017-08-07 09:29 PDT, Build Bot
2017-08-07 09:44 PDT, Manuel Rego Casasnovas
2017-08-07 10:52 PDT, Build Bot
2017-08-07 10:59 PDT, Build Bot
2017-08-07 11:19 PDT, Build Bot
2017-08-08 03:40 PDT, Manuel Rego Casasnovas
2017-08-08 10:15 PDT, Build Bot
2017-08-08 12:04 PDT, Build Bot
2017-08-08 23:11 PDT, Manuel Rego Casasnovas
2017-08-09 00:21 PDT, Build Bot
2017-08-09 00:34 PDT, Build Bot
2017-08-10 14:48 PDT, Ryosuke Niwa
2017-08-11 00:47 PDT, Manuel Rego Casasnovas