Merge r220639 - Composition underline color is always black

https://bugs.webkit.org/show_bug.cgi?id=174675 Reviewed by Ryosuke Niwa. Source/WebCore: This patch uses the current color of the text instead of black for the composition underline marker. This makes it visible in the case we have a black/dark background. Test: editing/composition-underline-color.html * editing/CompositionUnderline.h: (WebCore::CompositionUnderline::CompositionUnderline): Added new attribute compositionUnderlineColor. * rendering/InlineTextBox.cpp: (WebCore::InlineTextBox::paintCompositionUnderline): Use the text color if compositionUnderlineColor is TextColor. Source/WebKit: This patch uses the current color of the text instead of black for the composition underline marker. This makes it visible in the case we have a black/dark background. * UIProcess/Cocoa/WebViewImpl.mm: (WebKit::extractUnderlines): If NSUnderlineColorAttributeName is not present use text color for composition underline. (WebKit::WebViewImpl::setMarkedText): Use text color for composition underline in the plain text case. * UIProcess/gtk/InputMethodFilter.cpp: (WebKit::InputMethodFilter::handleKeyboardEventWithCompositionResults): Use text color for composition underline. (WebKit::InputMethodFilter::updatePreedit): Ditto. * WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::setCompositionForTesting): Ditto. Source/WebKitLegacy/mac: * WebView/WebHTMLView.mm: (extractUnderlines): If NSUnderlineColorAttributeName is not present use text color for composition underline. (-[WebHTMLView setMarkedText:selectedRange:]): Use text color for composition underline in the plain text case. Source/WebKitLegacy/win: * WebView.cpp: (WebView::setCompositionForTesting): Use text color for composition underline. LayoutTests: Added new test to check that the composition underline is using the text color. The test hides the text and the caret, so it only shows the composition underline and checks against an -expected-mismatch that the color of the composition marker is different. * editing/composition-underline-color-expected-mismatch.html: Added. * editing/composition-underline-color.html: Added.
WebKit · Aug 14, 2017 · ddc54aa · ddc54aa
1 parent d09d005
commit ddc54aa
Show file tree

Hide file tree

Showing 14 changed files with 183 additions and 17 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+2017-08-13  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Composition underline color is always black
+        https://bugs.webkit.org/show_bug.cgi?id=174675
+
+        Reviewed by Ryosuke Niwa.
+
+        Added new test to check that the composition underline
+        is using the text color.
+        The test hides the text and the caret, so it only shows
+        the composition underline and checks against an -expected-mismatch
+        that the color of the composition marker is different.
+
+        * editing/composition-underline-color-expected-mismatch.html: Added.
+        * editing/composition-underline-color.html: Added.
+
 2017-08-10  Nan Wang  <n_wang@apple.com>
 
         Layout test accessibility/press-target-uses-text-descendant-node.html is flaky.

diff --git a/LayoutTests/editing/composition-underline-color-expected-mismatch.html b/LayoutTests/editing/composition-underline-color-expected-mismatch.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<style>
+  div {
+    position: absolute;
+    top: 0;
+    left: 0;
+  }
+  #test {
+    color: magenta;
+    font: 20px/1 Monospace;
+    outline: none;
+  }
+  #overlapping-top {
+    background: white;
+    width: 100px;
+    height: 15px;
+  }
+  #overlapping-right {
+    background: white;
+    width: 50px;
+    height: 50px;
+    left: 50px;
+  }
+</style>
+<div contenteditable id="test"></div>
+<!-- The overlapping DIVs are hiding the "^^^^^" characters and the caret to show only the composition underline. -->
+<div id="overlapping-top"></div>
+<div id="overlapping-right"></div>
+<script>
+  document.getElementById("test").focus();
+  if (window.textInputController)
+    textInputController.setMarkedText("^^^^^", 5, 0);
+</script>
diff --git a/LayoutTests/editing/composition-underline-color.html b/LayoutTests/editing/composition-underline-color.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<style>
+  div {
+    position: absolute;
+    top: 0;
+    left: 0;
+  }
+  #test {
+    color: lime;
+    font: 20px/1 Monospace;
+    outline: none;
+  }
+  #overlapping-top {
+    background: white;
+    width: 100px;
+    height: 15px;
+  }
+  #overlapping-right {
+    background: white;
+    width: 50px;
+    height: 50px;
+    left: 50px;
+  }
+</style>
+<div contenteditable id="test"></div>
+<!-- The overlapping DIVs are hiding the "^^^^^" characters and the caret to show only the composition underline. -->
+<div id="overlapping-top"></div>
+<div id="overlapping-right"></div>
+<script>
+  document.getElementById("test").focus();
+  if (window.textInputController)
+    textInputController.setMarkedText("^^^^^", 5, 0);
+</script>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,23 @@
+2017-08-13  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Composition underline color is always black
+        https://bugs.webkit.org/show_bug.cgi?id=174675
+
+        Reviewed by Ryosuke Niwa.
+
+        This patch uses the current color of the text instead of black
+        for the composition underline marker.
+        This makes it visible in the case we have a black/dark background.
+
+        Test: editing/composition-underline-color.html
+
+        * editing/CompositionUnderline.h:
+        (WebCore::CompositionUnderline::CompositionUnderline):
+        Added new attribute compositionUnderlineColor.
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::paintCompositionUnderline):
+        Use the text color if compositionUnderlineColor is TextColor.
+
 2017-08-13  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] stop kinetic scrolling when a zero movement is reached

diff --git a/Source/WebCore/editing/CompositionUnderline.h b/Source/WebCore/editing/CompositionUnderline.h
@@ -29,21 +29,26 @@
 
 namespace WebCore {
 
+enum class CompositionUnderlineColor { GivenColor, TextColor };
+
 struct CompositionUnderline {
+
     CompositionUnderline()
     {
     }
 
-    CompositionUnderline(unsigned s, unsigned e, const Color& c, bool t) 
-        : startOffset(s)
-        , endOffset(e)
-        , color(c)
-        , thick(t)
+    CompositionUnderline(unsigned startOffset, unsigned endOffset, CompositionUnderlineColor compositionUnderlineColor, const Color& color, bool thick)
+        : startOffset(startOffset)
+        , endOffset(endOffset)
+        , compositionUnderlineColor(compositionUnderlineColor)
+        , color(color)
+        , thick(thick)
     {
     }
 
     unsigned startOffset { 0 };
     unsigned endOffset { 0 };
+    CompositionUnderlineColor compositionUnderlineColor { CompositionUnderlineColor::TextColor };
     Color color;
     bool thick { false };
 };

diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp
@@ -1007,7 +1007,7 @@ void InlineTextBox::paintCompositionUnderline(GraphicsContext& context, const Fl
     start += 1;
     width -= 2;
 
-    context.setStrokeColor(underline.color);
+    context.setStrokeColor(underline.compositionUnderlineColor == CompositionUnderlineColor::TextColor ? renderer().style().visitedDependentColor(CSSPropertyWebkitTextFillColor) : underline.color);
     context.setStrokeThickness(lineThickness);
     context.drawLineForText(FloatPoint(boxOrigin.x() + start, boxOrigin.y() + logicalHeight() - lineThickness), width, renderer().document().printing());
 }

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,26 @@
+2017-08-13  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Composition underline color is always black
+        https://bugs.webkit.org/show_bug.cgi?id=174675
+
+        Reviewed by Ryosuke Niwa.
+
+        This patch uses the current color of the text instead of black
+        for the composition underline marker.
+        This makes it visible in the case we have a black/dark background.
+
+        * UIProcess/Cocoa/WebViewImpl.mm:
+        (WebKit::extractUnderlines): If NSUnderlineColorAttributeName
+        is not present use text color for composition underline.
+        (WebKit::WebViewImpl::setMarkedText): Use text color
+        for composition underline in the plain text case.
+        * UIProcess/gtk/InputMethodFilter.cpp:
+        (WebKit::InputMethodFilter::handleKeyboardEventWithCompositionResults):
+        Use text color for composition underline.
+        (WebKit::InputMethodFilter::updatePreedit): Ditto.
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::setCompositionForTesting): Ditto.
+
 2017-08-13  Adrian Perez de Castro  <aperez@igalia.com>
 
         [WPE] Implement WebCore::standardUserAgent()

diff --git a/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm b/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm
@@ -4265,9 +4265,12 @@ static BOOL fileExists(NSString *path)
 
         if (NSNumber *style = [attrs objectForKey:NSUnderlineStyleAttributeName]) {
             WebCore::Color color = WebCore::Color::black;
-            if (NSColor *colorAttr = [attrs objectForKey:NSUnderlineColorAttributeName])
+            WebCore::CompositionUnderlineColor compositionUnderlineColor = WebCore::CompositionUnderlineColor::TextColor;
+            if (NSColor *colorAttr = [attrs objectForKey:NSUnderlineColorAttributeName]) {
                 color = WebCore::colorFromNSColor(colorAttr);
-            result.append(WebCore::CompositionUnderline(range.location, NSMaxRange(range), color, style.intValue > 1));
+                compositionUnderlineColor = WebCore::CompositionUnderlineColor::GivenColor;
+            }
+            result.append(WebCore::CompositionUnderline(range.location, NSMaxRange(range), compositionUnderlineColor, color, style.intValue > 1));
         }
 
         i = range.location + range.length;
@@ -4599,8 +4602,10 @@ static bool eventKeyCodeIsZeroOrNumLockOrFn(NSEvent *event)
         // FIXME: We ignore most attributes from the string, so an input method cannot specify e.g. a font or a glyph variation.
         text = [string string];
         underlines = extractUnderlines(string);
-    } else
+    } else {
         text = string;
+        underlines.append(WebCore::CompositionUnderline(0, [text length], WebCore::CompositionUnderlineColor::TextColor, WebCore::Color::black, false));
+    }
 
     if (inSecureInputState()) {
         // In password fields, we only allow ASCII dead keys, and don't allow inline input, matching NSSecureTextInputField.

diff --git a/Source/WebKit/UIProcess/gtk/InputMethodFilter.cpp b/Source/WebKit/UIProcess/gtk/InputMethodFilter.cpp
@@ -153,7 +153,7 @@ void InputMethodFilter::handleKeyboardEventWithCompositionResults(GdkEventKey* e
         m_page->confirmComposition(m_confirmedComposition, -1, 0);
 
     if (resultsToSend & Preedit && !m_preedit.isNull()) {
-        m_page->setComposition(m_preedit, Vector<CompositionUnderline>{ CompositionUnderline(0, m_preedit.length(), Color(1, 1, 1), false) },
+        m_page->setComposition(m_preedit, Vector<CompositionUnderline> { CompositionUnderline(0, m_preedit.length(), CompositionUnderlineColor::TextColor, Color(Color::black), false) },
             m_cursorOffset, m_cursorOffset, 0 /* replacement start */, 0 /* replacement end */);
     }
 }
@@ -260,7 +260,7 @@ void InputMethodFilter::updatePreedit()
     }
 #endif
     // FIXME: We should parse the PangoAttrList that we get from the IM context here.
-    m_page->setComposition(m_preedit, Vector<CompositionUnderline>{ CompositionUnderline(0, m_preedit.length(), Color(1, 1, 1), false) },
+    m_page->setComposition(m_preedit, Vector<CompositionUnderline> { CompositionUnderline(0, m_preedit.length(), CompositionUnderlineColor::TextColor, Color(Color::black), false) },
         m_cursorOffset, m_cursorOffset, 0 /* replacement start */, 0 /* replacement end */);
     m_preeditChanged = false;
 }

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -4669,7 +4669,7 @@ void WebPage::setCompositionForTesting(const String& compositionString, uint64_t
         return;
 
     Vector<CompositionUnderline> underlines;
-    underlines.append(CompositionUnderline(0, compositionString.length(), Color(Color::black), false));
+    underlines.append(CompositionUnderline(0, compositionString.length(), CompositionUnderlineColor::TextColor, Color(Color::black), false));
     frame.editor().setComposition(compositionString, underlines, from, from + length);
 }
 

diff --git a/Source/WebKitLegacy/mac/ChangeLog b/Source/WebKitLegacy/mac/ChangeLog
@@ -1,3 +1,16 @@
+2017-08-13  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Composition underline color is always black
+        https://bugs.webkit.org/show_bug.cgi?id=174675
+
+        Reviewed by Ryosuke Niwa.
+
+        * WebView/WebHTMLView.mm:
+        (extractUnderlines): If NSUnderlineColorAttributeName
+        is not present use text color for composition underline.
+        (-[WebHTMLView setMarkedText:selectedRange:]): Use text color
+        for composition underline in the plain text case.
+
 2017-08-08  Brady Eidson  <beidson@apple.com>
 
         Don't enable default icon loading in WK1 for apps linked against old SDKs.

diff --git a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
@@ -6945,9 +6945,12 @@ static void extractUnderlines(NSAttributedString *string, Vector<CompositionUnde
 
         if (NSNumber *style = [attrs objectForKey:NSUnderlineStyleAttributeName]) {
             Color color = Color::black;
-            if (NSColor *colorAttr = [attrs objectForKey:NSUnderlineColorAttributeName])
+            CompositionUnderlineColor compositionUnderlineColor = CompositionUnderlineColor::TextColor;
+            if (NSColor *colorAttr = [attrs objectForKey:NSUnderlineColorAttributeName]) {
                 color = colorFromNSColor(colorAttr);
-            result.append(CompositionUnderline(range.location, NSMaxRange(range), color, [style intValue] > 1));
+                compositionUnderlineColor = CompositionUnderlineColor::GivenColor;
+            }
+            result.append(CompositionUnderline(range.location, NSMaxRange(range), compositionUnderlineColor, color, [style intValue] > 1));
         }
 
         i = range.location + range.length;
@@ -6997,9 +7000,13 @@ - (void)setMarkedText:(id)string selectedRange:(NSRange)newSelRange
             replacementRange = NSRangeFromString(rangeString);
 
         extractUnderlines(string, underlines);
-    } else
-#endif
+    } else {
         text = string;
+        underlines.append(CompositionUnderline(0, [text length], CompositionUnderlineColor::TextColor, Color::black, false));
+    }
+#else
+    text = string;
+#endif
 
     if (replacementRange.location != NSNotFound)
         [[self _frame] _selectNSRange:replacementRange];

diff --git a/Source/WebKitLegacy/win/ChangeLog b/Source/WebKitLegacy/win/ChangeLog
@@ -1,3 +1,14 @@
+2017-08-13  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        Composition underline color is always black
+        https://bugs.webkit.org/show_bug.cgi?id=174675
+
+        Reviewed by Ryosuke Niwa.
+
+        * WebView.cpp:
+        (WebView::setCompositionForTesting): Use text color for
+        composition underline.
+
 2017-07-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Async image decoding for large images should be disabled after the first time a tile is painted

diff --git a/Source/WebKitLegacy/win/WebView.cpp b/Source/WebKitLegacy/win/WebView.cpp
@@ -7564,7 +7564,7 @@ HRESULT WebView::setCompositionForTesting(_In_ BSTR composition, UINT from, UINT
     String compositionStr = toString(composition);
 
     Vector<CompositionUnderline> underlines;
-    underlines.append(CompositionUnderline(0, compositionStr.length(), Color(Color::black), false));
+    underlines.append(CompositionUnderline(0, compositionStr.length(), CompositionUnderlineColor::TextColor, Color(Color::black), false));
     frame.editor().setComposition(compositionStr, underlines, from, from + length);
 
     return S_OK;