Attachment 370116 Details for Bug 197894 – Patch

[patch] Patch

bug-197894-20190517071103.patch (text/plain), 19.67 KB, created by Andres Gonzalez on 2019-05-17 07:11:04 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Andres Gonzalez

Created: 2019-05-17 07:11:04 PDT

Size: 19.67 KB

patch

obsolete

>Subversion Revision: 245007
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 26177d0480c0ee5f982bbef8fd38d16ff051bbee..05f9f32b21c8372d94c6f9384493e03a252b3d1a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-05-15  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 NOBODY (OOPS!).
>+
>+        Test: accessibility/set-selected-text-range-after-newline.html
>+
>+        * accessibility/AccessibilityRenderObject.cpp:
>+        (WebCore::AccessibilityRenderObject::setSelectedTextRange): avoids computing node more than once.
>+        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm: added iOS implementations.
>+        (-[WebAccessibilityObjectWrapper _accessibilitySelectedTextRange]):
>+        (-[WebAccessibilityObjectWrapper accessibilityReplaceRange:withText:]):
>+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
>+        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]): removed superfluous isNull check.
>+        * editing/TextIterator.cpp:
>+        (WebCore::CharacterIterator::range const): handling of runs consisting of a linebreak node.
>+        * editing/markup.cpp:
>+        (WebCore::createFragmentFromText): avoids creation of two elements to represent a linebreak.
>+
> 2019-05-07  Antti Koivisto  <antti@apple.com>
> 
>         <body> with overflow:hidden CSS is scrollable on iOS
>diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>index de2a0052b8345088f4cc1c7b572382b19f847a4c..221fb77dfccdc77602711ee8014471d2e784db05 100644
>--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>@@ -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);
>+        Node* 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));
>     }
>     
>diff --git a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>index c2009186ce53342a9996412d04ca697f19e27519..25f524bc0d96f08899a301929ede3e51ae37743e 100644
>--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>+++ b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>@@ -2497,11 +2497,13 @@ - (id)_stringForRange:(NSRange)range attributed:(BOOL)attributed
>     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 @@ - (NSRange)_accessibilitySelectedTextRange
> {
>     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 @@ - (void)_accessibilitySetSelectedTextRange:(NSRange)range
>     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
> {
>diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>index 23229cb53009e58d82588f9f340ff98fc677295c..0e0dcc45d597872c36c4cdca2918674cb5ba846d 100644
>--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>@@ -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>
>diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp
>index 23414a29564400e847c4515ba29a969ee8db2a81..a874108ae74dce78e54bb21509a01dce9e69ec9d 100644
>--- a/Source/WebCore/editing/TextIterator.cpp
>+++ b/Source/WebCore/editing/TextIterator.cpp
>@@ -1546,15 +1546,27 @@ CharacterIterator::CharacterIterator(Position start, Position end, TextIteratorB
> Ref<Range> CharacterIterator::range() const
> {
>     Ref<Range> range = m_underlyingIterator.range();
>-    if (!m_underlyingIterator.atEnd()) {
>-        if (m_underlyingIterator.text().length() <= 1) {
>-            ASSERT(m_runOffset == 0);
>+    Node* node = m_underlyingIterator.node();
>+    if (!m_underlyingIterator.atEnd() && node) {
>+        int runLength = m_underlyingIterator.text().length();
>+        if (runLength < 1) {
>+            ASSERT(!m_runOffset);
>+        } else if (runLength == 1) {
>+            ASSERT(!m_runOffset);
>+            if (range->startContainer().isCharacterDataNode())
>+                return range;
>+
>+            int endOffset = range->endOffset();
>+            Node* endNode = range->endContainer().traverseToChildAt(endOffset);
>+            if (endNode) {
>+                range->setStart(*node, 0);
>+                range->setEnd(*endNode, 0);
>+            }
>         } else {
>-            Node& node = range->startContainer();
>-            ASSERT(&node == &range->endContainer());
>+            ASSERT(node == &range->endContainer());
>             int offset = range->startOffset() + m_runOffset;
>-            range->setStart(node, offset);
>-            range->setEnd(node, offset + 1);
>+            range->setStart(*node, offset);
>+            range->setEnd(*node, offset + 1);
>         }
>     }
>     return range;
>diff --git a/Source/WebCore/editing/markup.cpp b/Source/WebCore/editing/markup.cpp
>index 00cb4e98f3070d54fad03ed407fb15a3494da85b..71c61e8456bd3cb892f0d6ba942111471c1dfc5c 100644
>--- a/Source/WebCore/editing/markup.cpp
>+++ b/Source/WebCore/editing/markup.cpp
>@@ -1114,12 +1114,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;
>     }
>@@ -1130,6 +1134,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);
>@@ -1148,8 +1158,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);
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 4fe89f7c75b2930a3b94933930b85c551474096b..6210b02b12240c1bc59615cc0f9bd00cb350b21b 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-15  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 NOBODY (OOPS!).
>+
>+        * WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm: iOS implementation of these methods.
>+        (WTR::AccessibilityUIElement::selectedTextRange):
>+        (WTR::AccessibilityUIElement::setSelectedTextRange):
>+        (WTR::AccessibilityUIElement::replaceTextInRange):
>+
> 2019-05-07  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [GTK][WPE] MiniBrowser: load about:blank for new web views in automation mode
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm b/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm
>index 7ed6cbd445eb993577cfeed4a76e8a41e26058a4..c60640e375f30cd250dff8fc4ef870b37c68804e 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm
>+++ b/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm
>@@ -55,6 +55,9 @@ - (NSArray *)elementsForRange:(NSRange)range;
> - (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)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 8b1d32e90313c2d2d32d911a040b43f940b75629..19a1d4186ae1bb4489705ef15c71f42d9f2ed162 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-05-15  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 NOBODY (OOPS!).
>+
>+        * accessibility/set-selected-text-range-after-newline-expected.txt: Added.
>+        * accessibility/set-selected-text-range-after-newline.html: Added.
>+
> 2019-05-07  Antti Koivisto  <antti@apple.com>
> 
>         <body> with overflow:hidden CSS is scrollable on iOS
>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
>index 0000000000000000000000000000000000000000..10d4527998e395430bdd3dd0db8d1567c7136e5d
>--- /dev/null
>+++ b/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..b955048747b3370f82c5286b4264dd79f6a0474c
>--- /dev/null
>+++ b/LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html
>@@ -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>
>diff --git a/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt b/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt
>index 8cad9e058aaf42254a8d8402c652b69234baedc0..1e6b19abe43368ea1a06873780b7211a13cc32cd 100644
>--- a/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt
>+++ b/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..10d4527998e395430bdd3dd0db8d1567c7136e5d
>--- /dev/null
>+++ b/LayoutTests/accessibility/set-selected-text-range-after-newline-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..b362534f99393d7527b6a8f2ea73fdf77ca639ac
>--- /dev/null
>+++ b/LayoutTests/accessibility/set-selected-text-range-after-newline.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 197894: 369892 | 369907 | 370014 | 370023 | 370028 | 370032 | 370116 | 370120 | 370121 | 370419 | 370435 | 370575 | 370964 | 370981 | 371007 | 371071