Created attachment 315988 [details]
Current output
Created attachment 315989 [details]
Expected output
Using the color of the font matches NSTextView so we should just do that. Created attachment 316082 [details]
Patch
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.
Created attachment 317402 [details]
Patch
Created attachment 317403 [details]
Patch
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. Created attachment 317405 [details]
Patch
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 Created attachment 317419 [details]
Patch
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 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 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 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 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 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 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 317426 [details]
Patch
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 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 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 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 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 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
Hm... your test appears to be blank on Mac. I wonder what happened there? Created attachment 317558 [details]
Patch
(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 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 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 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 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? 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. Alexey, does DumpRenderTree has a special behavior for drawing IME underline? Created attachment 317680 [details]
Patch
Thanks for the review, but we still need to decide what to do with the test in WK1. 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 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 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 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! I found a walkthrough on the internet: <https://chinese.yabla.com/type-chinese-characters.php?platform=mac>. 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.
Created attachment 317920 [details]
Patch
(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 on attachment 317920 [details] Patch Clearing flags on attachment: 317920 Committed r220639: <http://trac.webkit.org/changeset/220639> All reviewed patches have been landed. Closing bug. |
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.