Attachment 340958 Details for Bug 158115 – Fixes the bug

[patch] Fixes the bug

bug-158115-20180522000659.patch (text/plain), 13.91 KB, created by Ryosuke Niwa on 2018-05-22 00:07:00 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-05-22 00:07:00 PDT

Size: 13.91 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 232053)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,32 @@
>+2018-05-21  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        queryCommandState('underline') returns a wrong value after toggling typing style via execCommand('underline')
>+        https://bugs.webkit.org/show_bug.cgi?id=158115
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The bug was caused by EditingStyle::mergeTypingStyle failing to apply the removals of text decorations.
>+
>+        Because EditingStyle::overrideTypingStyleAt materializes text decoration changes to values of
>+        -webkit-text-decorations-in-effect at the time of applying typing text, EditingStyle::mergeTypingStyle
>+        has no way of differentiating when text decorations had been removed and when they were simply not present.
>+
>+        Fixed the bug the same way we fixed ApplyStyleCommand by keeping track of whether a given text-decoration
>+        had been added or removed using TextDecorationChange. For now, EditingStyle::overrideTypingStyleAt continues
>+        to materialize the changes to -webkit-text-decorations-in-effect since there are other codes that depend on
>+        this behavior.
>+
>+        Test: editing/execCommand/query-removed-text-decoration-with-typing-style.html
>+
>+        * editing/EditingStyle.cpp:
>+        (WebCore::applyTextDecorationChangeToValueList): Return a boolean indicating whether the value list is changed.
>+        (WebCore::EditingStyle::overrideTypingStyleAt): Store text decoration changes in addition to materializing
>+        those changes to -webkit-text-decorations-in-effect.
>+        (WebCore::EditingStyle::applyTextDecorationChanges): Extracted out of overrideTypingStyleAt. Also improved the
>+        efficiency of the code a little by avoiding to set the property value when no change is necessary.
>+        (WebCore::EditingStyle::mergeTypingStyle): Fixed the bug by applying text decoration changes.
>+        * editing/EditingStyle.h:
>+
> 2018-05-21  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         Use more C++17
>Index: Source/WebCore/editing/EditingStyle.cpp
>===================================================================
>--- Source/WebCore/editing/EditingStyle.cpp	(revision 232044)
>+++ Source/WebCore/editing/EditingStyle.cpp	(working copy)
>@@ -588,17 +588,21 @@ void EditingStyle::overrideWithStyle(con
>     return mergeStyle(style, OverrideValues);
> }
> 
>-static void applyTextDecorationChangeToValueList(CSSValueList& valueList, TextDecorationChange change, Ref<CSSPrimitiveValue>&& value)
>+static bool applyTextDecorationChangeToValueList(CSSValueList& valueList, TextDecorationChange change, Ref<CSSPrimitiveValue>&& value)
> {
>     switch (change) {
>     case TextDecorationChange::None:
>-        break;
>+        return false;
>     case TextDecorationChange::Add:
>+        if (valueList.hasValue(value.ptr()))
>+            return false;
>         valueList.append(WTFMove(value));
>-        break;
>+        return true;
>     case TextDecorationChange::Remove:
>+        if (!valueList.hasValue(value.ptr()))
>+            return false;
>         valueList.removeAll(&value.get());
>-        break;
>+        return true;
>     }
> }
> 
>@@ -610,31 +614,49 @@ void EditingStyle::overrideTypingStyleAt
> 
>     prepareToApplyAt(position, EditingStyle::PreserveWritingDirection);
> 
>+    // FIXME: Resolve the text decoration changes at the time of use. It's misleading to set both changes as well their effects in typing style.
>     auto underlineChange = style.underlineChange();
>     auto strikeThroughChange = style.strikeThroughChange();
>     if (underlineChange == TextDecorationChange::None && strikeThroughChange == TextDecorationChange::None)
>         return;
> 
>-    if (!m_mutableStyle)
>-        m_mutableStyle = MutableStyleProperties::create();
>+    if (underlineChange != TextDecorationChange::None)
>+        setUnderlineChange(underlineChange);
>+    
>+    if (strikeThroughChange != TextDecorationChange::None)
>+        setStrikeThroughChange(strikeThroughChange);
>+
>+    applyTextDecorationChanges(underlineChange, strikeThroughChange);
>+}
> 
>+void EditingStyle::applyTextDecorationChanges(TextDecorationChange underlineChange, TextDecorationChange strikeThroughChange)
>+{
>     auto& cssValuePool = CSSValuePool::singleton();
>     Ref<CSSPrimitiveValue> underline = cssValuePool.createIdentifierValue(CSSValueUnderline);
>     Ref<CSSPrimitiveValue> lineThrough = cssValuePool.createIdentifierValue(CSSValueLineThrough);
>-    RefPtr<CSSValue> value = m_mutableStyle->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
>-    RefPtr<CSSValueList> valueList;
>-    if (value && value->isValueList()) {
>-        valueList = downcast<CSSValueList>(*value).copy();
>-        applyTextDecorationChangeToValueList(*valueList, underlineChange, WTFMove(underline));
>-        applyTextDecorationChangeToValueList(*valueList, strikeThroughChange, WTFMove(lineThrough));
>+    
>+    RefPtr<CSSValue> currentValueList = m_mutableStyle ? m_mutableStyle->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) : nullptr;
>+    RefPtr<CSSValueList> newValueList;
>+    if (currentValueList && is<CSSValueList>(currentValueList)) {
>+        newValueList = downcast<CSSValueList>(*currentValueList).copy();
>+        bool didApplyUnderlineChange = applyTextDecorationChangeToValueList(*newValueList, underlineChange, WTFMove(underline));
>+        bool didApplyStrikeThroughChange = applyTextDecorationChangeToValueList(*newValueList, strikeThroughChange, WTFMove(lineThrough));
>+        if (!didApplyUnderlineChange && !didApplyStrikeThroughChange)
>+            return;
>     } else {
>-        valueList = CSSValueList::createSpaceSeparated();
>+        if (underlineChange != TextDecorationChange::Add && strikeThroughChange != TextDecorationChange::Add)
>+            return;
>+
>+        newValueList = CSSValueList::createSpaceSeparated();
>         if (underlineChange == TextDecorationChange::Add)
>-            valueList->append(WTFMove(underline));
>+            newValueList->append(WTFMove(underline));
>         if (strikeThroughChange == TextDecorationChange::Add)
>-            valueList->append(WTFMove(lineThrough));
>+            newValueList->append(WTFMove(lineThrough));
>+
>+        if (!m_mutableStyle)
>+            m_mutableStyle = MutableStyleProperties::create();
>     }
>-    m_mutableStyle->setProperty(CSSPropertyWebkitTextDecorationsInEffect, valueList.get());
>+    m_mutableStyle->setProperty(CSSPropertyWebkitTextDecorationsInEffect, WTFMove(newValueList));
> }
> 
> void EditingStyle::clear()
>@@ -1105,6 +1127,7 @@ void EditingStyle::mergeTypingStyle(Docu
>         return;
> 
>     mergeStyle(typingStyle->style(), OverrideValues);
>+    applyTextDecorationChanges(typingStyle->underlineChange(), typingStyle->strikeThroughChange());
> }
> 
> void EditingStyle::mergeInlineStyleOfElement(StyledElement* element, CSSPropertyOverrideMode mode, PropertiesToInclude propertiesToInclude)
>Index: Source/WebCore/editing/EditingStyle.h
>===================================================================
>--- Source/WebCore/editing/EditingStyle.h	(revision 232044)
>+++ Source/WebCore/editing/EditingStyle.h	(working copy)
>@@ -180,6 +180,7 @@ private:
>     void removeTextFillAndStrokeColorsIfNeeded(const RenderStyle*);
>     void setProperty(CSSPropertyID, const String& value, bool important = false);
>     void extractFontSizeDelta();
>+    void applyTextDecorationChanges(TextDecorationChange underlineChange, TextDecorationChange strikeThroughChange);
>     template<typename T> TriState triStateOfStyle(T& styleToCompare, ShouldIgnoreTextOnlyProperties) const;
>     bool conflictsWithInlineStyleOfElement(StyledElement*, RefPtr<MutableStyleProperties>* newInlineStyle, EditingStyle* extractedStyle) const;
>     void mergeInlineAndImplicitStyleOfElement(StyledElement&, CSSPropertyOverrideMode, PropertiesToInclude);
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 232044)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-05-21  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        queryCommandState('underline') returns a wrong value after toggling typing style via execCommand('underline')
>+        https://bugs.webkit.org/show_bug.cgi?id=158115
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added a test for querying underline and strikeThrough status when they were removed in the typing style.
>+
>+        * editing/execCommand/query-removed-text-decoration-with-typing-style-expected.txt: Added.
>+        * editing/execCommand/query-removed-text-decoration-with-typing-style.html: Added.
>+
> 2018-05-21  Chris Dumez  <cdumez@apple.com>
> 
>         File's structured serialization should serialize lastModified attribute
>Index: LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style-expected.txt
>===================================================================
>--- LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style-expected.txt	(nonexistent)
>+++ LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style-expected.txt	(working copy)
>@@ -0,0 +1,50 @@
>+This tests calling queryCommandState on text decorations that have been removed only in typing style.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+Testing underline
>+editor.innerHTML = "hello"; editor.focus(); document.execCommand("selectAll")
>+document.execCommand("bold"); document.execCommand("underline"); document.execCommand("strikethrough")
>+PASS editor.innerHTML is "<b><u><strike>hello</strike></u></b>"
>+getSelection().modify("move", "forward", "lineboundary")
>+PASS document.queryCommandState("bold") is true
>+PASS document.queryCommandState("underline") is true
>+PASS document.queryCommandState("strikethrough") is true
>+document.execCommand("underline", false, null)
>+PASS document.queryCommandState("bold") is true
>+PASS document.queryCommandState("underline") is false
>+PASS document.queryCommandState("strikethrough") is true
>+document.execCommand("bold", false, null)
>+PASS document.queryCommandState("bold") is false
>+PASS document.queryCommandState("underline") is false
>+PASS document.queryCommandState("strikethrough") is true
>+document.execCommand("insertText", false, " world")
>+PASS document.queryCommandState("bold") is false
>+PASS document.queryCommandState("underline") is false
>+PASS document.queryCommandState("strikethrough") is true
>+
>+Testing strikethrough
>+editor.innerHTML = "hello"; editor.focus(); document.execCommand("selectAll")
>+document.execCommand("bold"); document.execCommand("underline"); document.execCommand("strikethrough")
>+PASS editor.innerHTML is "<b><u><strike>hello</strike></u></b>"
>+getSelection().modify("move", "forward", "lineboundary")
>+PASS document.queryCommandState("bold") is true
>+PASS document.queryCommandState("strikethrough") is true
>+PASS document.queryCommandState("underline") is true
>+document.execCommand("strikethrough", false, null)
>+PASS document.queryCommandState("bold") is true
>+PASS document.queryCommandState("strikethrough") is false
>+PASS document.queryCommandState("underline") is true
>+document.execCommand("bold", false, null)
>+PASS document.queryCommandState("bold") is false
>+PASS document.queryCommandState("strikethrough") is false
>+PASS document.queryCommandState("underline") is true
>+document.execCommand("insertText", false, " world")
>+PASS document.queryCommandState("bold") is false
>+PASS document.queryCommandState("strikethrough") is false
>+PASS document.queryCommandState("underline") is true
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>Index: LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style.html
>===================================================================
>--- LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style.html	(nonexistent)
>+++ LayoutTests/editing/execCommand/query-removed-text-decoration-with-typing-style.html	(working copy)
>@@ -0,0 +1,49 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<div id="editor" contenteditable></div>
>+<script src="../../resources/js-test.js"></script>
>+<script>
>+
>+description('This tests calling queryCommandState on text decorations that have been removed only in typing style.');
>+
>+editor = document.getElementById('editor');
>+initialMarkup = '<u><s><b>hello</b></s></u>';
>+
>+function runTest(textDecorationToChange, otherTextDecoration) {
>+    debug('Testing ' + textDecorationToChange);
>+    evalAndLog('editor.innerHTML = "hello"; editor.focus(); document.execCommand("selectAll")');
>+    evalAndLog('document.execCommand("bold"); document.execCommand("underline"); document.execCommand("strikethrough")');
>+    shouldBeEqualToString('editor.innerHTML', '<b><u><strike>hello</strike></u></b>');
>+    evalAndLog('getSelection().modify("move", "forward", "lineboundary")');
>+
>+    shouldBeTrue('document.queryCommandState("bold")');
>+    shouldBeTrue(`document.queryCommandState("${textDecorationToChange}")`);
>+    shouldBeTrue(`document.queryCommandState("${otherTextDecoration}")`);
>+
>+    evalAndLog(`document.execCommand("${textDecorationToChange}", false, null)`);
>+    shouldBeTrue('document.queryCommandState("bold")');
>+    shouldBeFalse(`document.queryCommandState("${textDecorationToChange}")`);
>+    shouldBeTrue(`document.queryCommandState("${otherTextDecoration}")`);
>+
>+    evalAndLog('document.execCommand("bold", false, null)');
>+    shouldBeFalse('document.queryCommandState("bold")');
>+    shouldBeFalse(`document.queryCommandState("${textDecorationToChange}")`);
>+    shouldBeTrue(`document.queryCommandState("${otherTextDecoration}")`);
>+
>+    evalAndLog('document.execCommand("insertText", false, " world")');
>+    shouldBeFalse('document.queryCommandState("bold")');
>+    shouldBeFalse(`document.queryCommandState("${textDecorationToChange}")`);
>+    shouldBeTrue(`document.queryCommandState("${otherTextDecoration}")`);
>+}
>+
>+runTest('underline', 'strikethrough');
>+debug('');
>+runTest('strikethrough', 'underline');
>+
>+editor.style.display = 'none';
>+
>+</script>
>+</body>
>+</html>
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 158115: 279878 | 280058 | 340958