Attachment 371193 Details for Bug 198487 – Patch

[patch] Patch

bug-198487-20190603102130.patch (text/plain), 11.42 KB, created by Timothy Hatcher on 2019-06-03 10:21:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Timothy Hatcher

Created: 2019-06-03 10:21:30 PDT

Size: 11.42 KB

patch

obsolete

>Subversion Revision: 246023
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 196d2609c9e3d7207ffa92d1da2ea763f36a2105..7bbdddd525ead6064bb1d424883579604d80d7a7 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-06-03  Timothy Hatcher  <timothy@apple.com>
>+
>+        Tweak the text and underline color for data detected text.
>+        https://bugs.webkit.org/show_bug.cgi?id=198487
>+        rdar://problem/50667125
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Tests: Color.RGBToHSL API tests
>+
>+        * editing/cocoa/DataDetection.mm:
>+        (WebCore::DataDetection::detectContentInRange): Use currentcolor so semantic text colors work.
>+        Force the lightness of the underline color to the middle, and multiply the alpha by 38%,
>+        so the color will appear on light and dark backgrounds, since only one color can be specified.
>+        * platform/graphics/Color.cpp:
>+        (WebCore::Color::getHSL const): Return hue in [0...6) range to easily round-trip with makeRGBAFromHSLA().
>+
> 2019-06-01  Simon Fraser  <simon.fraser@apple.com>
> 
>         [Async overflow scroll] Flashing content when scrolling async overflow with a negative z-index child
>diff --git a/Source/WebCore/editing/cocoa/DataDetection.mm b/Source/WebCore/editing/cocoa/DataDetection.mm
>index ebe4bdc4210b67c058df01f1ae9d504e0a33b76e..bcb238e106d777883ede0f343ad46cf4c295a8dd 100644
>--- a/Source/WebCore/editing/cocoa/DataDetection.mm
>+++ b/Source/WebCore/editing/cocoa/DataDetection.mm
>@@ -580,12 +580,14 @@ NSArray *DataDetection::detectContentInRange(RefPtr<Range>& contextRange, DataDe
>             auto* parentNode = range->startContainer().parentNode();
>             if (!parentNode)
>                 continue;
>+
>             if (!is<Text>(range->startContainer()))
>                 continue;
>+
>             auto& currentTextNode = downcast<Text>(range->startContainer());
>             Document& document = currentTextNode.document();
>             String textNodeData;
>-            
>+
>             if (lastTextNodeToUpdate != &currentTextNode) {
>                 if (lastTextNodeToUpdate)
>                     lastTextNodeToUpdate->setData(lastNodeContent);
>@@ -594,12 +596,12 @@ NSArray *DataDetection::detectContentInRange(RefPtr<Range>& contextRange, DataDe
>                     textNodeData = currentTextNode.data().substring(0, range->startOffset());
>             } else
>                 textNodeData = currentTextNode.data().substring(contentOffset, range->startOffset() - contentOffset);
>-            
>+
>             if (!textNodeData.isEmpty()) {
>                 parentNode->insertBefore(Text::create(document, textNodeData), &currentTextNode);
>                 contentOffset = range->startOffset();
>             }
>-            
>+
>             // Create the actual anchor node and insert it before the current node.
>             textNodeData = currentTextNode.data().substring(range->startOffset(), range->endOffset() - range->startOffset());
>             Ref<Text> newTextNode = Text::create(document, textNodeData);
>@@ -608,35 +610,31 @@ NSArray *DataDetection::detectContentInRange(RefPtr<Range>& contextRange, DataDe
>             Ref<HTMLAnchorElement> anchorElement = HTMLAnchorElement::create(document);
>             anchorElement->setHref(correspondingURL);
>             anchorElement->setDir("ltr");
>+            anchorElement->setInlineStyleProperty(CSSPropertyColor, CSSValueCurrentcolor);
>+
>             if (shouldUseLightLinks) {
>                 document.updateStyleIfNeeded();
>+
>                 auto* renderStyle = parentNode->computedStyle();
>                 if (renderStyle) {
>                     auto textColor = renderStyle->visitedDependentColor(CSSPropertyColor);
>                     if (textColor.isValid()) {
>-                        double h = 0;
>-                        double s = 0;
>-                        double v = 0;
>-                        textColor.getHSV(h, s, v);
>-
>-                        // Set the alpha of the underline to 46% if the text color is white-ish (defined
>-                        // as having a saturation of less than 2% and a value/brightness or greater than
>-                        // 98%). Otherwise, set the alpha of the underline to 26%.
>-                        double overrideAlpha = (s < 0.02 && v > 0.98) ? 0.46 : 0.26;
>-                        auto underlineColor = Color(colorWithOverrideAlpha(textColor.rgb(), overrideAlpha));
>-
>-                        anchorElement->setInlineStyleProperty(CSSPropertyColor, textColor.cssText());
>+                        double hue, saturation, lightness;
>+                        textColor.getHSL(hue, saturation, lightness);
>+
>+                        // Force the lightness of the underline color to the middle, and multiply the alpha by 38%,
>+                        // so the color will appear on light and dark backgrounds, since only one color can be specified.
>+                        double overrideLightness = 0.5;
>+                        double overrideAlphaMultiplier = 0.38;
>+                        auto underlineColor = Color(makeRGBAFromHSLA(hue, saturation, overrideLightness, overrideAlphaMultiplier * textColor.alphaAsFloat()));
>+
>                         anchorElement->setInlineStyleProperty(CSSPropertyTextDecorationColor, underlineColor.cssText());
>                     }
>                 }
>-            } else if (is<StyledElement>(*parentNode)) {
>-                if (auto* style = downcast<StyledElement>(*parentNode).presentationAttributeStyle()) {
>-                    String color = style->getPropertyValue(CSSPropertyColor);
>-                    if (!color.isEmpty())
>-                        anchorElement->setInlineStyleProperty(CSSPropertyColor, color);
>-                }
>             }
>+
>             anchorElement->appendChild(WTFMove(newTextNode));
>+
>             // Add a special attribute to mark this URLification as the result of data detectors.
>             anchorElement->setAttributeWithoutSynchronization(x_apple_data_detectorsAttr, AtomicString("true", AtomicString::ConstructFromLiteral));
>             anchorElement->setAttributeWithoutSynchronization(x_apple_data_detectors_typeAttr, dataDetectorTypeForCategory(softLink_DataDetectorsCore_DDResultGetCategory(coreResult)));
>@@ -645,11 +643,12 @@ NSArray *DataDetection::detectContentInRange(RefPtr<Range>& contextRange, DataDe
>             parentNode->insertBefore(WTFMove(anchorElement), &currentTextNode);
> 
>             contentOffset = range->endOffset();
>-            
>+
>             lastNodeContent = currentTextNode.data().substring(range->endOffset(), currentTextNode.length() - range->endOffset());
>             lastTextNodeToUpdate = &currentTextNode;
>         }        
>     }
>+
>     if (lastTextNodeToUpdate)
>         lastTextNodeToUpdate->setData(lastNodeContent);
>     
>diff --git a/Source/WebCore/platform/graphics/Color.cpp b/Source/WebCore/platform/graphics/Color.cpp
>index 680a9cb66e7a58cb3999f30452be35b1c37576c1..c74d0219610c7bc738fc9c17b3551624c3987924 100644
>--- a/Source/WebCore/platform/graphics/Color.cpp
>+++ b/Source/WebCore/platform/graphics/Color.cpp
>@@ -542,7 +542,7 @@ void Color::getHSL(double& hue, double& saturation, double& lightness) const
> {
>     // http://en.wikipedia.org/wiki/HSL_color_space. This is a direct copy of
>     // the algorithm therein, although it's 360^o based and we end up wanting
>-    // [0...1) based. It's clearer if we stick to 360^o until the end.
>+    // [0...6) based. It's clearer if we stick to 360^o until the end.
>     double r = static_cast<double>(red()) / 255.0;
>     double g = static_cast<double>(green()) / 255.0;
>     double b = static_cast<double>(blue()) / 255.0;
>@@ -562,8 +562,8 @@ void Color::getHSL(double& hue, double& saturation, double& lightness) const
>     if (hue >= 360.0)
>         hue -= 360.0;
> 
>-    // makeRGBAFromHSLA assumes that hue is in [0...1).
>-    hue /= 360.0;
>+    // makeRGBAFromHSLA assumes that hue is in [0...6).
>+    hue /= 60.0;
> 
>     lightness = 0.5 * (max + min);
>     if (!chroma)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 288349033ceec728bdcd2b28d92272549e0c674c..d3583e725cb6c1d49cf51eccecf0bb6a8364bc21 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-06-03  Timothy Hatcher  <timothy@apple.com>
>+
>+        Tweak the text and underline color for data detected text.
>+        https://bugs.webkit.org/show_bug.cgi?id=198487
>+        rdar://problem/50667125
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WebCore/Color.cpp:
>+        (TestWebKitAPI::TEST): Added Color.RGBToHSL tests.
>+
> 2019-05-31  Sihui Liu  <sihui_liu@apple.com>
> 
>         TestWebKitAPI.WKWebView.LocalStorageProcessSuspends is flaky
>diff --git a/Tools/TestWebKitAPI/Tests/WebCore/Color.cpp b/Tools/TestWebKitAPI/Tests/WebCore/Color.cpp
>index 61af1c660d1e030723579c3c11e16c5a66652911..c1105e0327cfe5d040c5dd06b49592c0ed41f0be 100644
>--- a/Tools/TestWebKitAPI/Tests/WebCore/Color.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebCore/Color.cpp
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.
>+ * Copyright (C) 2011, 2012, 2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -144,6 +144,102 @@ TEST(Color, RGBToHSV_LightGray)
>     EXPECT_DOUBLE_EQ(0.75294117647058822, v);
> }
> 
>+TEST(Color, RGBToHSL_White)
>+{
>+    Color color = Color::white;
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(0, saturation);
>+    EXPECT_DOUBLE_EQ(1, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_Black)
>+{
>+    Color color = Color::black;
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(0, saturation);
>+    EXPECT_DOUBLE_EQ(0, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_Red)
>+{
>+    Color color(255, 0, 0);
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(1, saturation);
>+    EXPECT_DOUBLE_EQ(0.5, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_Green)
>+{
>+    Color color(0, 255, 0);
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(2, hue);
>+    EXPECT_DOUBLE_EQ(1, saturation);
>+    EXPECT_DOUBLE_EQ(0.5, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_Blue)
>+{
>+    Color color(0, 0, 255);
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(4, hue);
>+    EXPECT_DOUBLE_EQ(1, saturation);
>+    EXPECT_DOUBLE_EQ(0.5, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_DarkGray)
>+{
>+    Color color = Color::darkGray;
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(0, saturation);
>+    EXPECT_DOUBLE_EQ(0.50196078431372548, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_Gray)
>+{
>+    Color color = Color::gray;
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(0, saturation);
>+    EXPECT_DOUBLE_EQ(0.62745098039215685, lightness);
>+}
>+
>+TEST(Color, RGBToHSL_LightGray)
>+{
>+    Color color = Color::lightGray;
>+
>+    double hue, saturation, lightness;
>+    color.getHSL(hue, saturation, lightness);
>+
>+    EXPECT_DOUBLE_EQ(0, hue);
>+    EXPECT_DOUBLE_EQ(0, saturation);
>+    EXPECT_DOUBLE_EQ(0.75294117647058822, lightness);
>+}
>+
> TEST(Color, Validity)
> {
>     Color invalidColor;

Actions: View | Formatted Diff | Diff

Attachments on bug 198487: 371193