Title: [245912] trunk
Revision
245912
Author
[email protected]
Date
2019-05-30 17:02:59 -0700 (Thu, 30 May 2019)

Log Message

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>

Patch by Andres Gonzalez <[email protected]> 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:

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (245911 => 245912)


--- trunk/LayoutTests/ChangeLog	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/LayoutTests/ChangeLog	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1,3 +1,18 @@
+2019-05-30  Andres Gonzalez  <[email protected]>
+
+        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  <[email protected]>
 
         Web Inspector: Audit: tests are unable to get the current Audit version

Added: trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt (0 => 245912)


--- trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt	2019-05-31 00:02:59 UTC (rev 245912)
@@ -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
+

Added: trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html (0 => 245912)


--- trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html	                        (rev 0)
+++ trunk/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html	2019-05-31 00:02:59 UTC (rev 245912)
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script src=""
+</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=""
+</body>
+</html>

Modified: trunk/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt (245911 => 245912)


--- trunk/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt	2019-05-31 00:02:59 UTC (rev 245912)
@@ -4,7 +4,7 @@
 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

Added: trunk/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt (0 => 245912)


--- trunk/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt	2019-05-31 00:02:59 UTC (rev 245912)
@@ -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
+

Added: trunk/LayoutTests/accessibility/set-selected-text-range-after-newline.html (0 => 245912)


--- trunk/LayoutTests/accessibility/set-selected-text-range-after-newline.html	                        (rev 0)
+++ trunk/LayoutTests/accessibility/set-selected-text-range-after-newline.html	2019-05-31 00:02:59 UTC (rev 245912)
@@ -0,0 +1,40 @@
+<html>
+<head>
+<script src=""
+</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=""
+</body>
+</html>

Modified: trunk/LayoutTests/platform/win/TestExpectations (245911 => 245912)


--- trunk/LayoutTests/platform/win/TestExpectations	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/LayoutTests/platform/win/TestExpectations	2019-05-31 00:02:59 UTC (rev 245912)
@@ -3394,6 +3394,7 @@
 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 ]

Modified: trunk/Source/WebCore/ChangeLog (245911 => 245912)


--- trunk/Source/WebCore/ChangeLog	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/ChangeLog	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1,3 +1,41 @@
+2019-05-30  Andres Gonzalez  <[email protected]>
+
+        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  <[email protected]>
 
         [Web GPU] Vertex Buffers/Input State API updates

Modified: trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp (245911 => 245912)


--- trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1621,9 +1621,10 @@
         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));
     }
     

Modified: trunk/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm (245911 => 245912)


--- trunk/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm	2019-05-31 00:02:59 UTC (rev 245912)
@@ -2497,11 +2497,13 @@
     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 @@
 {
     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 @@
     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
 {

Modified: trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm (245911 => 245912)


--- trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm	2019-05-31 00:02:59 UTC (rev 245912)
@@ -2742,8 +2742,6 @@
         }
         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>

Modified: trunk/Source/WebCore/editing/Editing.cpp (245911 => 245912)


--- trunk/Source/WebCore/editing/Editing.cpp	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/editing/Editing.cpp	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1121,6 +1121,16 @@
     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 };
 }
 

Modified: trunk/Source/WebCore/editing/markup.cpp (245911 => 245912)


--- trunk/Source/WebCore/editing/markup.cpp	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Source/WebCore/editing/markup.cpp	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1115,12 +1115,16 @@
     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 @@
         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 @@
         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);

Modified: trunk/Tools/ChangeLog (245911 => 245912)


--- trunk/Tools/ChangeLog	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Tools/ChangeLog	2019-05-31 00:02:59 UTC (rev 245912)
@@ -1,3 +1,19 @@
+2019-05-30  Andres Gonzalez  <[email protected]>
+
+        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  <[email protected]>
 
         IsoHeaps don't notice uncommitted VA becoming the first eligible.

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm (245911 => 245912)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm	2019-05-31 00:00:09 UTC (rev 245911)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm	2019-05-31 00:02:59 UTC (rev 245912)
@@ -55,6 +55,9 @@
 - (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 @@
 
 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::setSelectedTextRange(unsigned location, unsigned length)
 {
-    return false;
+    [m_element _accessibilitySetSelectedTextRange:NSMakeRange(location, length)];
+    return true;
 }
 
 void AccessibilityUIElement::increment()
@@ -1145,9 +1151,9 @@
     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)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to