Inserting an image, selecting, underlining, and then deleting leaves the typing style...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Nov 2017 23:26:55 +0000 (23:26 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Nov 2017 23:26:55 +0000 (23:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=179431

Reviewed by Ryosuke Niwa.

Source/WebCore:

When inserting an image element, selecting it, underlining the selection, deleting, and then inserting text, we
crash on a debug assert. This codepath was exercised by an API test added in <https://trac.webkit.org/r224512>.
This assertion happens due to the following sequence of events:
1. DeleteSelectionCommand::saveTypingStyleState computes a typing style.
2. In doing so, it calls into EditingStyle::init, which observes that "-webkit-text-decorations-in-effect" is
   present and appends "text-decoration" with an identical CSS value to the EditingStyle's mutable style
   properties.
3. DeleteSelectionCommand::calculateTypingStyleAfterDelete sets the current selection's typing style to the
   above typing style.
4. Later on, when we try to insert text, we compute the StyleChange using the above typing style, which calls
   into reconcileTextDecorationProperties.
5. reconcileTextDecorationProperties debug asserts that "-webkit-text-decorations-in-effect" and
   "text-decoration" don't coexist on the EditingStyle's (i.e. the typing style's) mutable properties; since (2)
   added both properties, this assertion fires.

It appears that step (2) shouldn't be adding "text-decoration" in addition to EditingStyle's mutable style
properties, since doing so would violate the requirements of reconcileTextDecorationProperties. As such, we can
tweak EditingStyle::init to *replace* the "-webkit-text-decorations-in-effect" property with "text-decoration"
instead; this matches the behavior of reconcileTextDecorationProperties, and ensures that we only have the
"text-decorations" property when we try to insert text in step (4).

Test: editing/execCommand/underline-selection-containing-image.html

* editing/EditingStyle.cpp:
(WebCore::EditingStyle::init):

LayoutTests:

Adds a new layout test to fix a debug assertion. See WebCore/ChangeLog for more details. Additionally
rebaselines a few existing tests that serialize markup strings to include `text-decoration: none;`.

* editing/execCommand/underline-selection-containing-image-expected.txt: Added.
* editing/execCommand/underline-selection-containing-image.html: Added.
* editing/pasteboard/data-transfer-get-data-on-drop-rich-text-expected.txt:
* fast/events/before-input-events-prevent-drag-and-drop-expected.txt:
* fast/events/input-events-paste-rich-datatransfer-expected.txt:
* fast/events/ondrop-text-html-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224649 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/underline-selection-containing-image-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/underline-selection-containing-image.html [new file with mode: 0644]
LayoutTests/editing/pasteboard/data-transfer-get-data-on-drop-rich-text-expected.txt
LayoutTests/fast/events/before-input-events-prevent-drag-and-drop-expected.txt
LayoutTests/fast/events/input-events-paste-rich-datatransfer-expected.txt
LayoutTests/fast/events/ondrop-text-html-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/editing/EditingStyle.cpp

index 49fca39..b5869e8 100644 (file)
@@ -1,3 +1,20 @@
+2017-11-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Inserting an image, selecting, underlining, and then deleting leaves the typing style with both "-webkit-text-decorations-in-effect" and "text-decoration"
+        https://bugs.webkit.org/show_bug.cgi?id=179431
+
+        Reviewed by Ryosuke Niwa.
+
+        Adds a new layout test to fix a debug assertion. See WebCore/ChangeLog for more details. Additionally
+        rebaselines a few existing tests that serialize markup strings to include `text-decoration: none;`.
+
+        * editing/execCommand/underline-selection-containing-image-expected.txt: Added.
+        * editing/execCommand/underline-selection-containing-image.html: Added.
+        * editing/pasteboard/data-transfer-get-data-on-drop-rich-text-expected.txt:
+        * fast/events/before-input-events-prevent-drag-and-drop-expected.txt:
+        * fast/events/input-events-paste-rich-datatransfer-expected.txt:
+        * fast/events/ondrop-text-html-expected.txt:
+
 2017-11-09  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: support undo/redo of insertAdjacentHTML
diff --git a/LayoutTests/editing/execCommand/underline-selection-containing-image-expected.txt b/LayoutTests/editing/execCommand/underline-selection-containing-image-expected.txt
new file mode 100644 (file)
index 0000000..31c1f0d
--- /dev/null
@@ -0,0 +1 @@
+The underlined text should be 'foo': 'foo'
diff --git a/LayoutTests/editing/execCommand/underline-selection-containing-image.html b/LayoutTests/editing/execCommand/underline-selection-containing-image.html
new file mode 100644 (file)
index 0000000..3e7b546
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<body contenteditable></body>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+document.body.focus();
+document.execCommand("InsertHTML", true, "<img></img>");
+document.execCommand("SelectAll");
+document.execCommand("Underline");
+document.execCommand("InsertText", true, "foo");
+document.body.textContent = `The underlined text should be 'foo': '${document.querySelector("U").textContent}'`;
+</script>
index 05b8781..19aa3ee 100644 (file)
@@ -5,7 +5,7 @@ Rich text
         "text/plain": ""
     },
     "drop": {
-        "text/html": "<!DOCTYPE html><strong style=\"font-family: -apple-system; font-size: 150px; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: nowrap; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; color: purple;\">Rich text</strong>",
+        "text/html": "<!DOCTYPE html><strong style=\"font-family: -apple-system; font-size: 150px; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: nowrap; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none; color: purple;\">Rich text</strong>",
         "text/plain": "Rich text"
     }
 }
index e062f4e..e963e0f 100644 (file)
@@ -7,4 +7,4 @@ Destination:
 
 HTML content:
 
-<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: monospace; font-size: 108px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: center; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none;">input events</span>
+<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: monospace; font-size: 108px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: center; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none; display: inline !important; float: none;">input events</span>
index 175632c..65dac21 100644 (file)
@@ -2,7 +2,7 @@ To manually test this, copy and paste into the first contenteditable. The follow
 
 destination after pasting (text/html):
 | <b>
-|   style="caret-color: rgb(255, 0, 0); color: rgb(255, 0, 0); font-family: -webkit-standard; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"
+|   style="caret-color: rgb(255, 0, 0); color: rgb(255, 0, 0); font-family: -webkit-standard; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;"
 |   "LayoutTests"
 |   <i>
 |     "are"
index 47616bc..42dccf2 100644 (file)
@@ -1,4 +1,4 @@
 CONSOLE MESSAGE: line 21: text/plain: This test verifies that we can get text/html from the drag object during an ondrop event. 
-CONSOLE MESSAGE: line 23: text/html: <span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-size: medium; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; display: inline !important; float: none;">This test verifies that we can get text/html from the drag object during an ondrop event.<span class="Apple-converted-space"> </span></span>
+CONSOLE MESSAGE: line 23: text/html: <span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-size: medium; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none; display: inline !important; float: none;">This test verifies that we can get text/html from the drag object during an ondrop event.<span class="Apple-converted-space"> </span></span>
 This test verifies that we can get text/html from the drag object during an ondrop event. This test requires DRT.
 PASS
index a21161a..dcf634a 100644 (file)
@@ -1,3 +1,36 @@
+2017-11-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Inserting an image, selecting, underlining, and then deleting leaves the typing style with both "-webkit-text-decorations-in-effect" and "text-decoration"
+        https://bugs.webkit.org/show_bug.cgi?id=179431
+
+        Reviewed by Ryosuke Niwa.
+
+        When inserting an image element, selecting it, underlining the selection, deleting, and then inserting text, we
+        crash on a debug assert. This codepath was exercised by an API test added in <https://trac.webkit.org/r224512>.
+        This assertion happens due to the following sequence of events:
+        1. DeleteSelectionCommand::saveTypingStyleState computes a typing style.
+        2. In doing so, it calls into EditingStyle::init, which observes that "-webkit-text-decorations-in-effect" is
+           present and appends "text-decoration" with an identical CSS value to the EditingStyle's mutable style
+           properties.
+        3. DeleteSelectionCommand::calculateTypingStyleAfterDelete sets the current selection's typing style to the
+           above typing style.
+        4. Later on, when we try to insert text, we compute the StyleChange using the above typing style, which calls
+           into reconcileTextDecorationProperties.
+        5. reconcileTextDecorationProperties debug asserts that "-webkit-text-decorations-in-effect" and
+           "text-decoration" don't coexist on the EditingStyle's (i.e. the typing style's) mutable properties; since (2)
+           added both properties, this assertion fires.
+
+        It appears that step (2) shouldn't be adding "text-decoration" in addition to EditingStyle's mutable style
+        properties, since doing so would violate the requirements of reconcileTextDecorationProperties. As such, we can
+        tweak EditingStyle::init to *replace* the "-webkit-text-decorations-in-effect" property with "text-decoration"
+        instead; this matches the behavior of reconcileTextDecorationProperties, and ensures that we only have the
+        "text-decorations" property when we try to insert text in step (4).
+
+        Test: editing/execCommand/underline-selection-containing-image.html
+
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::init):
+
 2017-11-09  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: support undo/redo of insertAdjacentHTML
index ee4eda5..b88737d 100644 (file)
@@ -455,8 +455,10 @@ void EditingStyle::init(Node* node, PropertiesToInclude propertiesToInclude)
     if (propertiesToInclude == EditingPropertiesInEffect) {
         if (RefPtr<CSSValue> value = backgroundColorInEffect(node))
             m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText());
-        if (RefPtr<CSSValue> value = computedStyleAtPosition.propertyValue(CSSPropertyWebkitTextDecorationsInEffect))
+        if (RefPtr<CSSValue> value = computedStyleAtPosition.propertyValue(CSSPropertyWebkitTextDecorationsInEffect)) {
             m_mutableStyle->setProperty(CSSPropertyTextDecoration, value->cssText());
+            m_mutableStyle->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
+        }
     }
 
     if (node && node->computedStyle()) {