Inserting a newline in contenteditable causes two characters to be added instead...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 May 2019 00:02:59 +0000 (00:02 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 May 2019 00:02:59 +0000 (00:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197894
<rdar://problem/49700998>

Patch by Andres Gonzalez <andresg_22@apple.com> on 2019-05-30
Reviewed by Wenson Hsieh and Chris Fleizach.

Source/WebCore:

There were two issues with inserting a newline character at the end of
a line that caused problems for accessibility:
- the first '\n' inserted after text would result in two line breaks
inserted instead of one. createFragmentFromText in markup.cpp was
splitting the string "\n" into two empty strings and creating a <div>
and a <br> respectively. Then the emission code would emit a '\n' for
the empty div and another for the <br>.
- the second problem is a consequence of <rdar://problem/5192593> and
the workaround is the change in editing.cpp in the function
visiblePositionForIndexUsingCharacterIterator, similar to what is done
in VisibleUnits.cpp for nextBoundary.
The rest of the changes in this patch are accessibility changes to
execute the layout tests.

Tests: accessibility/ios-simulator/set-selected-text-range-after-newline.html
       accessibility/set-selected-text-range-after-newline.html

* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::setSelectedTextRange):
* accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
(-[WebAccessibilityObjectWrapper stringForRange:]):
(-[WebAccessibilityObjectWrapper _accessibilitySelectedTextRange]):
(-[WebAccessibilityObjectWrapper accessibilityReplaceRange:withText:]):
* accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
(-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]):
* editing/Editing.cpp:
(WebCore::visiblePositionForIndexUsingCharacterIterator):
* editing/markup.cpp:
(WebCore::createFragmentFromText):

Tools:

iOS implementation of several AccessibilityUIElement methods to execute
LayoutTests.

* WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:
(WTR::AccessibilityUIElement::selectedTextRange):
(WTR::AccessibilityUIElement::setSelectedTextRange):
(WTR::AccessibilityUIElement::replaceTextInRange):

LayoutTests:

* accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt: Added.
* accessibility/ios-simulator/set-selected-text-range-after-newline.html: Added.
* accessibility/ios-simulator/text-marker-list-item-expected.txt:
* accessibility/set-selected-text-range-after-newline-expected.txt: Added.
* accessibility/set-selected-text-range-after-newline.html: Added.
* platform/win/TestExpectations:

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html [new file with mode: 0644]
LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt
LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/set-selected-text-range-after-newline.html [new file with mode: 0644]
LayoutTests/platform/win/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AccessibilityRenderObject.cpp
Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
Source/WebCore/editing/Editing.cpp
Source/WebCore/editing/markup.cpp
Tools/ChangeLog
Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm

index 9af2679..6004325 100644 (file)
@@ -1,3 +1,18 @@
+2019-05-30  Andres Gonzalez  <andresg_22@apple.com>
+
+        Inserting a newline in contenteditable causes two characters to be added instead of one
+        https://bugs.webkit.org/show_bug.cgi?id=197894
+        <rdar://problem/49700998>
+
+        Reviewed by Wenson Hsieh and Chris Fleizach.
+
+        * accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt: Added.
+        * accessibility/ios-simulator/set-selected-text-range-after-newline.html: Added.
+        * accessibility/ios-simulator/text-marker-list-item-expected.txt:
+        * accessibility/set-selected-text-range-after-newline-expected.txt: Added.
+        * accessibility/set-selected-text-range-after-newline.html: Added.
+        * platform/win/TestExpectations:
+
 2019-05-30  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Audit: tests are unable to get the current Audit version
diff --git a/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt b/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt
new file mode 100644 (file)
index 0000000..10d4527
--- /dev/null
@@ -0,0 +1,10 @@
+hello
+world
+PASS text.selectedTextRange became '{5, 0}'
+There must be only one [newline] between hello and world: hello[newline]world
+PASS text.selectedTextRange became '{6, 0}'
+The text after the newline should be world: world
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html b/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html
new file mode 100644 (file)
index 0000000..8db656f
--- /dev/null
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+
+<div id="content" contenteditable tabindex="0">helloworld</div>
+
+<div id="console"></div>
+
+<script>
+    if (window.accessibilityController) {
+        window.jsTestIsAsync = true;
+
+        var content = document.getElementById("content");
+        content.focus();
+
+        var text = accessibilityController.focusedElement;
+        text.setSelectedTextRange(5, 0);
+        shouldBecomeEqual("text.selectedTextRange", "'{5, 0}'", function() {
+            text.replaceTextInRange("\n", 5, 0);
+
+            var t = text.stringForRange(0, 11);
+            t = t.replace(/(?:\r\n|\r|\n)/g, '[newline]');
+            debug("There must be only one [newline] between hello and world: " + t);
+
+            text.setSelectedTextRange(6, 0);
+            shouldBecomeEqual("text.selectedTextRange", "'{6, 0}'", function() {
+                var t = text.stringForRange(6, 5);
+                t = t.replace(/(?:\r\n|\r|\n)/g, '[newline]');
+                debug("The text after the newline should be world: " + t);
+
+                finishJSTest();
+            });
+        });
+    }
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 8cad9e0..1e6b19a 100644 (file)
@@ -4,7 +4,7 @@ This test ensures that when asking for the string for a range, it includes the l
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-FAIL text should be 1. item 1. Was 1. .
+PASS text is '1. item 1'
 PASS successfullyParsed is true
 
 TEST COMPLETE
diff --git a/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt b/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt
new file mode 100644 (file)
index 0000000..10d4527
--- /dev/null
@@ -0,0 +1,10 @@
+hello
+world
+PASS text.selectedTextRange became '{5, 0}'
+There must be only one [newline] between hello and world: hello[newline]world
+PASS text.selectedTextRange became '{6, 0}'
+The text after the newline should be world: world
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/set-selected-text-range-after-newline.html b/LayoutTests/accessibility/set-selected-text-range-after-newline.html
new file mode 100644 (file)
index 0000000..4ffc2e3
--- /dev/null
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+
+<div id="content" contenteditable tabindex="0">helloworld</div>
+
+<div id="console"></div>
+
+<script>
+    if (window.accessibilityController) {
+        window.jsTestIsAsync = true;
+
+        var content = document.getElementById("content");
+        content.focus();
+
+        var text = accessibilityController.focusedElement;
+        text.setSelectedTextRange(5, 0);
+        shouldBecomeEqual("text.selectedTextRange", "'{5, 0}'", function() {
+            text.replaceTextInRange("\n", 5, 0);
+
+            var t = text.stringForRange(0, 11);
+            t = t.replace(/(?:\r\n|\r|\n)/g, '[newline]');
+            debug("There must be only one [newline] between hello and world: " + t);
+
+            text.setSelectedTextRange(6, 0);
+            shouldBecomeEqual("text.selectedTextRange", "'{6, 0}'", function() {
+                var t = text.stringForRange(6, 5);
+                t = t.replace(/(?:\r\n|\r|\n)/g, '[newline]');
+                debug("The text after the newline should be world: " + t);
+
+                finishJSTest();
+            });
+        });
+    }
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
index 754a4a3..d9b3f7a 100644 (file)
@@ -3394,6 +3394,7 @@ legacy-animation-engine/animations/animation-delay-changed.html [ Skip ]
 js/dom/create-lots-of-workers.html [ Crash ]
 # Timeouts tracked in webkit.org/b/160447.
 accessibility/set-selected-text-range-contenteditable.html [ Skip ]
+accessibility/set-selected-text-range-after-newline.html [ Skip ]
 crypto/crypto-key-algorithm-gc.html [ Skip ]
 crypto/crypto-key-usages-gc.html [ Skip ]
 editing/deleting/delete-emoji.html [ Skip ]
index 25f9187..a368a5f 100644 (file)
@@ -1,3 +1,41 @@
+2019-05-30  Andres Gonzalez  <andresg_22@apple.com>
+
+        Inserting a newline in contenteditable causes two characters to be added instead of one
+        https://bugs.webkit.org/show_bug.cgi?id=197894
+        <rdar://problem/49700998>
+
+        Reviewed by Wenson Hsieh and Chris Fleizach.
+
+        There were two issues with inserting a newline character at the end of 
+        a line that caused problems for accessibility:
+        - the first '\n' inserted after text would result in two line breaks 
+        inserted instead of one. createFragmentFromText in markup.cpp was 
+        splitting the string "\n" into two empty strings and creating a <div> 
+        and a <br> respectively. Then the emission code would emit a '\n' for 
+        the empty div and another for the <br>.
+        - the second problem is a consequence of <rdar://problem/5192593> and 
+        the workaround is the change in editing.cpp in the function
+        visiblePositionForIndexUsingCharacterIterator, similar to what is done
+        in VisibleUnits.cpp for nextBoundary.
+        The rest of the changes in this patch are accessibility changes to 
+        execute the layout tests.
+
+        Tests: accessibility/ios-simulator/set-selected-text-range-after-newline.html
+               accessibility/set-selected-text-range-after-newline.html
+
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::setSelectedTextRange):
+        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
+        (-[WebAccessibilityObjectWrapper stringForRange:]):
+        (-[WebAccessibilityObjectWrapper _accessibilitySelectedTextRange]):
+        (-[WebAccessibilityObjectWrapper accessibilityReplaceRange:withText:]):
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
+        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]):
+        * editing/Editing.cpp:
+        (WebCore::visiblePositionForIndexUsingCharacterIterator):
+        * editing/markup.cpp:
+        (WebCore::createFragmentFromText):
+
 2019-05-30  Justin Fan  <justin_fan@apple.com>
 
         [Web GPU] Vertex Buffers/Input State API updates
index 468e558..5655009 100644 (file)
@@ -1621,9 +1621,10 @@ void AccessibilityRenderObject::setSelectedTextRange(const PlainTextRange& range
         HTMLTextFormControlElement& textControl = downcast<RenderTextControl>(*m_renderer).textFormControlElement();
         textControl.setSelectionRange(range.start, range.start + range.length);
     } else {
-        ASSERT(node());
-        VisiblePosition start = visiblePositionForIndexUsingCharacterIterator(*node(), range.start);
-        VisiblePosition end = visiblePositionForIndexUsingCharacterIterator(*node(), range.start + range.length);
+        auto node = this->node();
+        ASSERT(node);
+        VisiblePosition start = visiblePositionForIndexUsingCharacterIterator(*node, range.start);
+        VisiblePosition end = visiblePositionForIndexUsingCharacterIterator(*node, range.start + range.length);
         m_renderer->frame().selection().setSelection(VisibleSelection(start, end), FrameSelection::defaultSetSelectionOptions(UserTriggered));
     }
     
index c200918..25f524b 100644 (file)
@@ -2497,11 +2497,13 @@ static void AXAttributedStringAppendText(NSMutableAttributedString* attrString,
     return [self _stringFromStartMarker:startMarker toEndMarker:endMarker attributed:attributed];
 }
 
-
-// A convenience method for getting the text of a NSRange. Currently used only by DRT.
+// A convenience method for getting the text of a NSRange.
 - (NSString *)stringForRange:(NSRange)range
 {
-    return [self _stringForRange:range attributed:NO];
+    if (![self _prepareAccessibilityCall])
+        return nil;
+
+    return m_object->stringForRange([self _convertToDOMRange:range]);
 }
 
 - (NSAttributedString *)attributedStringForRange:(NSRange)range
@@ -2525,11 +2527,11 @@ static void AXAttributedStringAppendText(NSMutableAttributedString* attrString,
 {
     if (![self _prepareAccessibilityCall] || !m_object->isTextControl())
         return NSMakeRange(NSNotFound, 0);
-    
+
     PlainTextRange textRange = m_object->selectedTextRange();
     if (textRange.isNull())
         return NSMakeRange(NSNotFound, 0);
-    return NSMakeRange(textRange.start, textRange.length);    
+    return NSMakeRange(textRange.start, textRange.length);
 }
 
 - (void)_accessibilitySetSelectedTextRange:(NSRange)range
@@ -2540,6 +2542,14 @@ static void AXAttributedStringAppendText(NSMutableAttributedString* attrString,
     m_object->setSelectedTextRange(PlainTextRange(range.location, range.length));
 }
 
+- (BOOL)accessibilityReplaceRange:(NSRange)range withText:(NSString *)string
+{
+    if (![self _prepareAccessibilityCall])
+        return NO;
+
+    return m_object->replaceTextInRange(string, PlainTextRange(range));
+}
+
 // A convenience method for getting the accessibility objects of a NSRange. Currently used only by DRT.
 - (NSArray *)elementsForRange:(NSRange)range
 {
index 23229cb..0e0dcc4 100644 (file)
@@ -2742,8 +2742,6 @@ IGNORE_WARNINGS_END
         }
         if ([attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute]) {
             PlainTextRange textRange = m_object->selectedTextRange();
-            if (textRange.isNull())
-                return [NSValue valueWithRange:NSMakeRange(0, 0)];
             return [NSValue valueWithRange:NSMakeRange(textRange.start, textRange.length)];
         }
         // TODO: Get actual visible range. <rdar://problem/4712101>
index ea67834..25450fc 100644 (file)
@@ -1121,6 +1121,16 @@ VisiblePosition visiblePositionForIndexUsingCharacterIterator(Node& node, int in
     range->selectNodeContents(node);
     CharacterIterator it(range.get());
     it.advance(index - 1);
+
+    if (!it.atEnd() && it.text()[0] == '\n') {
+        // FIXME: workaround for collapsed range (where only start position is correct) emitted for some emitted newlines (see rdar://5192593)
+        auto range = it.range();
+        if (range->startPosition() == range->endPosition()) {
+            it.advance(1);
+            return VisiblePosition(it.range()->startPosition());
+        }
+    }
+
     return { it.atEnd() ? range->endPosition() : it.range()->endPosition(), UPSTREAM };
 }
 
index bfc3fb7..d9ecf15 100644 (file)
@@ -1115,12 +1115,16 @@ Ref<DocumentFragment> createFragmentFromText(Range& context, const String& text)
     string.replace("\r\n", "\n");
     string.replace('\r', '\n');
 
+    auto createHTMLBRElement = [&document]() {
+        auto element = HTMLBRElement::create(document);
+        element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
+        return element;
+    };
+
     if (contextPreservesNewline(context)) {
         fragment->appendChild(document.createTextNode(string));
         if (string.endsWith('\n')) {
-            auto element = HTMLBRElement::create(document);
-            element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
-            fragment->appendChild(element);
+            fragment->appendChild(createHTMLBRElement());
         }
         return fragment;
     }
@@ -1131,6 +1135,12 @@ Ref<DocumentFragment> createFragmentFromText(Range& context, const String& text)
         return fragment;
     }
 
+    if (string.length() == 1 && string[0] == '\n') {
+        // This is a single newline char, thus just create one HTMLBRElement.
+        fragment->appendChild(createHTMLBRElement());
+        return fragment;
+    }
+
     // Break string into paragraphs. Extra line breaks turn into empty paragraphs.
     Node* blockNode = enclosingBlock(context.firstNode());
     Element* block = downcast<Element>(blockNode);
@@ -1149,8 +1159,7 @@ Ref<DocumentFragment> createFragmentFromText(Range& context, const String& text)
         RefPtr<Element> element;
         if (s.isEmpty() && i + 1 == numLines) {
             // For last line, use the "magic BR" rather than a P.
-            element = HTMLBRElement::create(document);
-            element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
+            element = createHTMLBRElement();
         } else if (useLineBreak) {
             element = HTMLBRElement::create(document);
             fillContainerFromString(fragment, s);
index 5fb6173..9a77b25 100644 (file)
@@ -1,3 +1,19 @@
+2019-05-30  Andres Gonzalez  <andresg_22@apple.com>
+
+        Inserting a newline in contenteditable causes two characters to be added instead of one
+        https://bugs.webkit.org/show_bug.cgi?id=197894
+        <rdar://problem/49700998>
+
+        Reviewed by Wenson Hsieh and Chris Fleizach.
+
+        iOS implementation of several AccessibilityUIElement methods to execute
+        LayoutTests.
+        * WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:
+        (WTR::AccessibilityUIElement::selectedTextRange):
+        (WTR::AccessibilityUIElement::setSelectedTextRange):
+        (WTR::AccessibilityUIElement::replaceTextInRange):
+
 2019-05-30  Keith Miller  <keith_miller@apple.com>
 
         IsoHeaps don't notice uncommitted VA becoming the first eligible.
index 7ed6cbd..c60640e 100644 (file)
@@ -55,6 +55,9 @@ typedef void (*AXPostedNotificationCallback)(id element, NSString* notification,
 - (NSString *)selectionRangeString;
 - (CGPoint)accessibilityClickPoint;
 - (void)accessibilityModifySelection:(WebCore::TextGranularity)granularity increase:(BOOL)increase;
+- (NSRange)_accessibilitySelectedTextRange;
+- (void)_accessibilitySetSelectedTextRange:(NSRange)range;
+- (BOOL)accessibilityReplaceRange:(NSRange)range withText:(NSString *)string;
 - (void)accessibilitySetPostedNotificationCallback:(AXPostedNotificationCallback)function withContext:(void*)context;
 - (CGFloat)_accessibilityMinValue;
 - (CGFloat)_accessibilityMaxValue;
@@ -836,7 +839,9 @@ void AccessibilityUIElement::scrollToMakeVisibleWithSubFocus(int x, int y, int w
 
 JSRetainPtr<JSStringRef> AccessibilityUIElement::selectedTextRange()
 {
-    return createEmptyJSString();
+    NSRange range = [m_element _accessibilitySelectedTextRange];
+    NSMutableString *rangeDescription = [NSMutableString stringWithFormat:@"{%lu, %lu}", static_cast<unsigned long>(range.location), static_cast<unsigned long>(range.length)];
+    return [rangeDescription createJSStringRef];
 }
 
 bool AccessibilityUIElement::setSelectedVisibleTextRange(AccessibilityTextMarkerRange*)
@@ -846,7 +851,8 @@ bool AccessibilityUIElement::setSelectedVisibleTextRange(AccessibilityTextMarker
 
 bool AccessibilityUIElement::setSelectedTextRange(unsigned location, unsigned length)
 {
-    return false;
+    [m_element _accessibilitySetSelectedTextRange:NSMakeRange(location, length)];
+    return true;
 }
 
 void AccessibilityUIElement::increment()
@@ -1145,9 +1151,9 @@ RefPtr<AccessibilityTextMarker> AccessibilityUIElement::startTextMarkerForBounds
     return nullptr;
 }
     
-bool AccessibilityUIElement::replaceTextInRange(JSStringRef, int, int)
+bool AccessibilityUIElement::replaceTextInRange(JSStringRef string, int location, int length)
 {
-    return false;
+    return [m_element accessibilityReplaceRange:NSMakeRange(location, length) withText:[NSString stringWithJSStringRef:string]];
 }
 
 RefPtr<AccessibilityTextMarker> AccessibilityUIElement::textMarkerForPoint(int x, int y)