Attachment 323865 Details for Bug 170779 – Patch

[patch] Patch

patch.txt (text/plain), 13.53 KB, created by Nikita Vasilyev on 2017-10-15 20:24:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2017-10-15 20:24:59 PDT

Size: 13.53 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ceee791..3af18b3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2017-10-15  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Modify CSS number values with up key and down key
>+        https://bugs.webkit.org/show_bug.cgi?id=170779
>+        <rdar://problem/33170633>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add tests for WI.incrementElementValue defined in WebInspectorUI/UserInterface/Views/EditingSupport.js.
>+
>+        * inspector/unit-tests/editing-support-expected.txt: Added.
>+        * inspector/unit-tests/editing-support.html: Added.
>+
> 2017-10-14  Antoine Quint  <graouts@apple.com>
> 
>         Remove all Web Animations code
>diff --git a/LayoutTests/inspector/unit-tests/editing-support-expected.txt b/LayoutTests/inspector/unit-tests/editing-support-expected.txt
>new file mode 100644
>index 0000000..040abb1
>--- /dev/null
>+++ b/LayoutTests/inspector/unit-tests/editing-support-expected.txt
>@@ -0,0 +1,13 @@
>+Testing that WI.incrementElementValue works for numeric values.
>+
>+
>+== Running test suite: WI.incrementElementValue
>+-- Running test case: incrementElementValue.numeric
>+PASS: Should increment by 0.1.
>+PASS: Should decrement by 1.
>+PASS: Should increment by 10.
>+PASS: Should decrement by 100.
>+
>+-- Running test case: incrementElementValue.nonNumeric
>+PASS: Text should not change.
>+
>diff --git a/LayoutTests/inspector/unit-tests/editing-support.html b/LayoutTests/inspector/unit-tests/editing-support.html
>new file mode 100644
>index 0000000..7c9245d
>--- /dev/null
>+++ b/LayoutTests/inspector/unit-tests/editing-support.html
>@@ -0,0 +1,65 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
>+<script>
>+function test() {
>+    let suite = InspectorTest.createAsyncSuite("WI.incrementElementValue");
>+
>+    suite.addTestCase({
>+        name: "incrementElementValue.numeric",
>+        description: "Ensure that WI.incrementElementValue increments and decrements a numeric value.",
>+        test(resolve, reject) {
>+            let span = document.createElement("span");
>+            span.contentEditable = true;
>+            span.textContent = "font-size: 10px;";
>+            document.body.append(span);
>+
>+            let selection = window.getSelection();
>+            let textNode = span.firstChild;
>+            selection.setBaseAndExtent(textNode, "font-size: ".length, textNode, "font-size: 10px".length);
>+
>+            WI.incrementElementValue(span, 0.1);
>+            InspectorTest.expectEqual(span.textContent, "font-size: 10.1px;", `Should increment by 0.1.`);
>+
>+            WI.incrementElementValue(span, -1);
>+            InspectorTest.expectEqual(span.textContent, "font-size: 9.1px;", `Should decrement by 1.`);
>+
>+            WI.incrementElementValue(span, 10);
>+            InspectorTest.expectEqual(span.textContent, "font-size: 19.1px;", `Should increment by 10.`);
>+
>+            WI.incrementElementValue(span, -100);
>+            InspectorTest.expectEqual(span.textContent, "font-size: -80.9px;", `Should decrement by 100.`);
>+
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "incrementElementValue.nonNumeric",
>+        description: "Ensure that WI.incrementElementValue does not change non-numeric values.",
>+        test(resolve, reject) {
>+            let span = document.createElement("span");
>+            span.contentEditable = true;
>+            span.textContent = "background-color";
>+            document.body.append(span);
>+
>+            let selection = window.getSelection();
>+            let textNode = span.firstChild;
>+            selection.setBaseAndExtent(textNode, "background".length, textNode, "background-".length);
>+
>+            WI.incrementElementValue(span, 1);
>+            InspectorTest.expectEqual(span.textContent, "background-color", `Text should not change.`);
>+
>+            resolve();
>+        }
>+    });
>+
>+    suite.runTestCasesAndFinish();
>+}
>+</script>
>+</head>
>+<body onload="runTest()">
>+    <p>Testing that WI.incrementElementValue works for numeric values.</p>
>+</body>
>+</html>
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index ba85aad..6495b9b 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,26 @@
>+2017-10-15  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Modify CSS number values with up key and down key
>+        https://bugs.webkit.org/show_bug.cgi?id=170779
>+        <rdar://problem/33170633>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Up key increments a number, Down key decrements it.
>+
>+        Holding modifier keys changes the step value:
>+        - Option modifies the value by 0.1
>+        - Shift modifies the value by 10
>+        - Command modifies the value by 100
>+
>+        * UserInterface/Test.html:
>+        * UserInterface/Views/EditingSupport.js:
>+        (WI.incrementElementValue):
>+        Abstract away incrementElementValue into a public method.
>+
>+        * UserInterface/Views/SpreadsheetTextField.js:
>+        (WI.SpreadsheetTextField.prototype._handleKeyDown):
>+
> 2017-10-14  Nikita Vasilyev  <nvasilyev@apple.com>
> 
>         REGRESSION (r223310): Web Inspector: Class list container is transparent and unreadable
>diff --git a/Source/WebInspectorUI/UserInterface/Test.html b/Source/WebInspectorUI/UserInterface/Test.html
>index dea731a..89ab41c 100644
>--- a/Source/WebInspectorUI/UserInterface/Test.html
>+++ b/Source/WebInspectorUI/UserInterface/Test.html
>@@ -216,6 +216,7 @@
>     <script src="Workers/Formatter/FormatterContentBuilder.js"></script>
>     <script src="Views/CodeMirrorAdditions.js"></script>
>     <script src="Views/CodeMirrorFormatters.js"></script>
>+    <script src="Views/EditingSupport.js"></script>
>     <script src="Views/View.js"></script>
> 
>     <script type="text/javascript">
>diff --git a/Source/WebInspectorUI/UserInterface/Views/EditingSupport.js b/Source/WebInspectorUI/UserInterface/Views/EditingSupport.js
>index c39beb4..bb56a4a 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/EditingSupport.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/EditingSupport.js
>@@ -237,64 +237,10 @@ WI.startEditing = function(element, config)
>             else if (event.ctrlKey)
>                 modifyValue /= 10;
> 
>-            let selection = element.ownerDocument.defaultView.getSelection();
>-            if (!selection.rangeCount)
>+            let modified = WI.incrementElementValue(element, modifyValue);
>+            if (!modified)
>                 return;
> 
>-            let range = selection.getRangeAt(0);
>-            if (!range.commonAncestorContainer.isSelfOrDescendant(element))
>-                return false;
>-
>-            let wordRange = range.startContainer.rangeOfWord(range.startOffset, WI.EditingSupport.StyleValueDelimiters, element);
>-            let word = wordRange.toString();
>-            let wordPrefix = "";
>-            let wordSuffix = "";
>-            let nonNumberInWord = /[^\d-\.]+/.exec(word);
>-            if (nonNumberInWord) {
>-                let nonNumberEndOffset = nonNumberInWord.index + nonNumberInWord[0].length;
>-                if (range.startOffset > wordRange.startOffset + nonNumberInWord.index && nonNumberEndOffset < word.length && range.startOffset !== wordRange.startOffset) {
>-                    wordPrefix = word.substring(0, nonNumberEndOffset);
>-                    word = word.substring(nonNumberEndOffset);
>-                } else {
>-                    wordSuffix = word.substring(nonNumberInWord.index);
>-                    word = word.substring(0, nonNumberInWord.index);
>-                }
>-            }
>-
>-            let matches = WI.EditingSupport.CSSNumberRegex.exec(word);
>-            if (!matches || matches.length !== 4)
>-                return;
>-
>-            let replacement = matches[1] + (Math.round((parseFloat(matches[2]) + modifyValue) * 100) / 100) + matches[3];
>-
>-            selection.removeAllRanges();
>-            selection.addRange(wordRange);
>-            document.execCommand("insertText", false, wordPrefix + replacement + wordSuffix);
>-
>-            let container = range.commonAncestorContainer;
>-            let startOffset = range.startOffset;
>-            // This check is for the situation when the cursor is in the space between the
>-            // opening quote of the attribute and the first character. In that spot, the
>-            // commonAncestorContainer is actually the entire attribute node since `="` is
>-            // added as a simple text node. Since the opening quote is immediately before
>-            // the attribute, the node for that attribute must be the next sibling and the
>-            // text of the attribute's value must be the first child of that sibling.
>-            if (container.parentNode.classList.contains("editing")) {
>-                container = container.nextSibling.firstChild;
>-                startOffset = 0;
>-            }
>-            startOffset += wordPrefix.length;
>-
>-            if (!container)
>-                return;
>-
>-            let replacementSelectionRange = document.createRange();
>-            replacementSelectionRange.setStart(container, startOffset);
>-            replacementSelectionRange.setEnd(container, startOffset + replacement.length);
>-
>-            selection.removeAllRanges();
>-            selection.addRange(replacementSelectionRange);
>-
>             if (typeof config.numberCommitHandler === "function")
>                 config.numberCommitHandler(element, getContent(element), oldText, context, moveDirection);
> 
>@@ -328,6 +274,69 @@ WI.startEditing = function(element, config)
>     };
> };
> 
>+WI.incrementElementValue = function(element, delta)
>+{
>+    let selection = element.ownerDocument.defaultView.getSelection();
>+    if (!selection.rangeCount)
>+        return false;
>+
>+    let range = selection.getRangeAt(0);
>+    if (!range.commonAncestorContainer.isSelfOrDescendant(element))
>+        return false;
>+
>+    let wordRange = range.startContainer.rangeOfWord(range.startOffset, WI.EditingSupport.StyleValueDelimiters, element);
>+    let word = wordRange.toString();
>+    let wordPrefix = "";
>+    let wordSuffix = "";
>+    let nonNumberInWord = /[^\d-\.]+/.exec(word);
>+    if (nonNumberInWord) {
>+        let nonNumberEndOffset = nonNumberInWord.index + nonNumberInWord[0].length;
>+        if (range.startOffset > wordRange.startOffset + nonNumberInWord.index && nonNumberEndOffset < word.length && range.startOffset !== wordRange.startOffset) {
>+            wordPrefix = word.substring(0, nonNumberEndOffset);
>+            word = word.substring(nonNumberEndOffset);
>+        } else {
>+            wordSuffix = word.substring(nonNumberInWord.index);
>+            word = word.substring(0, nonNumberInWord.index);
>+        }
>+    }
>+
>+    let matches = WI.EditingSupport.CSSNumberRegex.exec(word);
>+    if (!matches || matches.length !== 4)
>+        return false;
>+
>+    let replacement = matches[1] + (Math.round((parseFloat(matches[2]) + delta) * 100) / 100) + matches[3];
>+
>+    selection.removeAllRanges();
>+    selection.addRange(wordRange);
>+    document.execCommand("insertText", false, wordPrefix + replacement + wordSuffix);
>+
>+    let container = range.commonAncestorContainer;
>+    let startOffset = range.startOffset;
>+    // This check is for the situation when the cursor is in the space between the
>+    // opening quote of the attribute and the first character. In that spot, the
>+    // commonAncestorContainer is actually the entire attribute node since `="` is
>+    // added as a simple text node. Since the opening quote is immediately before
>+    // the attribute, the node for that attribute must be the next sibling and the
>+    // text of the attribute's value must be the first child of that sibling.
>+    if (container.parentNode.classList.contains("editing") && container.nextSibling) {
>+        container = container.nextSibling.firstChild;
>+        startOffset = 0;
>+    }
>+    startOffset += wordPrefix.length;
>+
>+    if (!container)
>+        return false;
>+
>+    let replacementSelectionRange = document.createRange();
>+    replacementSelectionRange.setStart(container, startOffset);
>+    replacementSelectionRange.setEnd(container, startOffset + replacement.length);
>+
>+    selection.removeAllRanges();
>+    selection.addRange(replacementSelectionRange);
>+
>+    return true;
>+};
>+
> WI.EditingSupport = {
>     StyleValueDelimiters: " \xA0\t\n\"':;,/()",
>     CSSNumberRegex: /(.*?)(-?(?:\d+(?:\.\d+)?|\.\d+))(.*)/,
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js
>index b96bf54..049d6a0 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js
>@@ -225,6 +225,28 @@ WI.SpreadsheetTextField = class SpreadsheetTextField
>             return;
>         }
> 
>+        if (event.key === "ArrowUp" || event.key === "ArrowDown") {
>+            let delta = 1;
>+            if (event.metaKey)
>+                delta = 100;
>+            else if (event.shiftKey)
>+                delta = 10;
>+            else if (event.altKey)
>+                delta = 0.1;
>+
>+            if (event.key === "ArrowDown")
>+                delta = -delta;
>+
>+            let didModify = WI.incrementElementValue(this._element, delta);
>+            if (!didModify)
>+                return;
>+
>+            event.stop();
>+
>+            if (this._delegate && typeof this._delegate.spreadsheetTextFieldDidChange === "function")
>+                this._delegate.spreadsheetTextFieldDidChange(this);
>+        }
>+
>         if (event.key === "Escape") {
>             event.stop();
>             this._discardChange();

Flags:

mattbaker: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 170779: 323845 | 323846 | 323865 | 323874