Changeset 245912 in webkit


Ignore:
Timestamp:
May 30, 2019 5:02:59 PM (5 years ago)
Author:
commit-queue@webkit.org
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 <Andres Gonzalez> 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:
Location:
trunk
Files:
4 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r245909 r245912  
     12019-05-30  Andres Gonzalez  <andresg_22@apple.com>
     2
     3        Inserting a newline in contenteditable causes two characters to be added instead of one
     4        https://bugs.webkit.org/show_bug.cgi?id=197894
     5        <rdar://problem/49700998>
     6
     7        Reviewed by Wenson Hsieh and Chris Fleizach.
     8
     9        * accessibility/ios-simulator/set-selected-text-range-after-newline-expected.txt: Added.
     10        * accessibility/ios-simulator/set-selected-text-range-after-newline.html: Added.
     11        * accessibility/ios-simulator/text-marker-list-item-expected.txt:
     12        * accessibility/set-selected-text-range-after-newline-expected.txt: Added.
     13        * accessibility/set-selected-text-range-after-newline.html: Added.
     14        * platform/win/TestExpectations:
     15
    1162019-05-30  Devin Rousso  <drousso@apple.com>
    217

  • trunk/LayoutTests/accessibility/ios-simulator/text-marker-list-item-expected.txt

    r187904 r245912  
    55
    66
    7 FAIL text should be 1. item 1. Was 1. .
     7PASS text is '1. item 1'
    88PASS successfullyParsed is true
    99

  • trunk/LayoutTests/platform/win/TestExpectations

    r245733 r245912  
    33953395# Timeouts tracked in webkit.org/b/160447.
    33963396accessibility/set-selected-text-range-contenteditable.html [ Skip ]
     3397accessibility/set-selected-text-range-after-newline.html [ Skip ]
    33973398crypto/crypto-key-algorithm-gc.html [ Skip ]
    33983399crypto/crypto-key-usages-gc.html [ Skip ]

  • trunk/Source/WebCore/ChangeLog

    r245905 r245912  
     12019-05-30  Andres Gonzalez  <andresg_22@apple.com>
     2
     3        Inserting a newline in contenteditable causes two characters to be added instead of one
     4        https://bugs.webkit.org/show_bug.cgi?id=197894
     5        <rdar://problem/49700998>
     6
     7        Reviewed by Wenson Hsieh and Chris Fleizach.
     8
     9        There were two issues with inserting a newline character at the end of
     10        a line that caused problems for accessibility:
     11        - the first '\n' inserted after text would result in two line breaks
     12        inserted instead of one. createFragmentFromText in markup.cpp was
     13        splitting the string "\n" into two empty strings and creating a <div>
     14        and a <br> respectively. Then the emission code would emit a '\n' for
     15        the empty div and another for the <br>.
     16        - the second problem is a consequence of <rdar://problem/5192593> and
     17        the workaround is the change in editing.cpp in the function
     18        visiblePositionForIndexUsingCharacterIterator, similar to what is done
     19        in VisibleUnits.cpp for nextBoundary.
     20        The rest of the changes in this patch are accessibility changes to
     21        execute the layout tests.
     22
     23        Tests: accessibility/ios-simulator/set-selected-text-range-after-newline.html
     24               accessibility/set-selected-text-range-after-newline.html
     25
     26        * accessibility/AccessibilityRenderObject.cpp:
     27        (WebCore::AccessibilityRenderObject::setSelectedTextRange):
     28        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
     29        (-[WebAccessibilityObjectWrapper stringForRange:]):
     30        (-[WebAccessibilityObjectWrapper _accessibilitySelectedTextRange]):
     31        (-[WebAccessibilityObjectWrapper accessibilityReplaceRange:withText:]):
     32        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
     33        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]):
     34        * editing/Editing.cpp:
     35        (WebCore::visiblePositionForIndexUsingCharacterIterator):
     36        * editing/markup.cpp:
     37        (WebCore::createFragmentFromText):
     38
    1392019-05-30  Justin Fan  <justin_fan@apple.com>
    240

  • trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

    r245823 r245912  
    16221622        textControl.setSelectionRange(range.start, range.start + range.length);
    16231623    } else {
    1624         ASSERT(node());
    1625         VisiblePosition start = visiblePositionForIndexUsingCharacterIterator(*node(), range.start);
    1626         VisiblePosition end = visiblePositionForIndexUsingCharacterIterator(*node(), range.start + range.length);
     1624        auto node = this->node();
     1625        ASSERT(node);
     1626        VisiblePosition start = visiblePositionForIndexUsingCharacterIterator(*node, range.start);
     1627        VisiblePosition end = visiblePositionForIndexUsingCharacterIterator(*node, range.start + range.length);
    16271628        m_renderer->frame().selection().setSelection(VisibleSelection(start, end), FrameSelection::defaultSetSelectionOptions(UserTriggered));
    16281629    }

  • trunk/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

    r244983 r245912  
    24982498}
    24992499
    2500 
    2501 // A convenience method for getting the text of a NSRange. Currently used only by DRT.
     2500// A convenience method for getting the text of a NSRange.
    25022501- (NSString *)stringForRange:(NSRange)range
    25032502{
    2504     return [self _stringForRange:range attributed:NO];
     2503    if (![self _prepareAccessibilityCall])
     2504        return nil;
     2505
     2506    return m_object->stringForRange([self _convertToDOMRange:range]);
    25052507}
    25062508
    … …  
    25262528    if (![self _prepareAccessibilityCall] || !m_object->isTextControl())
    25272529        return NSMakeRange(NSNotFound, 0);
    2528    
     2530
    25292531    PlainTextRange textRange = m_object->selectedTextRange();
    25302532    if (textRange.isNull())
    25312533        return NSMakeRange(NSNotFound, 0);
    2532     return NSMakeRange(textRange.start, textRange.length);   
     2534    return NSMakeRange(textRange.start, textRange.length);
    25332535}
    25342536
    … …  
    25392541   
    25402542    m_object->setSelectedTextRange(PlainTextRange(range.location, range.length));
     2543}
     2544
     2545- (BOOL)accessibilityReplaceRange:(NSRange)range withText:(NSString *)string
     2546{
     2547    if (![self _prepareAccessibilityCall])
     2548        return NO;
     2549
     2550    return m_object->replaceTextInRange(string, PlainTextRange(range));
    25412551}
    25422552

  • trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

    r244983 r245912  
    27432743        if ([attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute]) {
    27442744            PlainTextRange textRange = m_object->selectedTextRange();
    2745             if (textRange.isNull())
    2746                 return [NSValue valueWithRange:NSMakeRange(0, 0)];
    27472745            return [NSValue valueWithRange:NSMakeRange(textRange.start, textRange.length)];
    27482746        }

  • trunk/Source/WebCore/editing/Editing.cpp

    r243163 r245912  
    11221122    CharacterIterator it(range.get());
    11231123    it.advance(index - 1);
     1124
     1125    if (!it.atEnd() && it.text()[0] == '\n') {
     1126        // FIXME: workaround for collapsed range (where only start position is correct) emitted for some emitted newlines (see rdar://5192593)
     1127        auto range = it.range();
     1128        if (range->startPosition() == range->endPosition()) {
     1129            it.advance(1);
     1130            return VisiblePosition(it.range()->startPosition());
     1131        }
     1132    }
     1133
    11241134    return { it.atEnd() ? range->endPosition() : it.range()->endPosition(), UPSTREAM };
    11251135}

  • trunk/Source/WebCore/editing/markup.cpp

    r245637 r245912  
    11161116    string.replace('\r', '\n');
    11171117
     1118    auto createHTMLBRElement = [&document]() {
     1119        auto element = HTMLBRElement::create(document);
     1120        element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
     1121        return element;
     1122    };
     1123
    11181124    if (contextPreservesNewline(context)) {
    11191125        fragment->appendChild(document.createTextNode(string));
    11201126        if (string.endsWith('\n')) {
    1121             auto element = HTMLBRElement::create(document);
    1122             element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
    1123             fragment->appendChild(element);
     1127            fragment->appendChild(createHTMLBRElement());
    11241128        }
    11251129        return fragment;
    … …  
    11291133    if (string.find('\n') == notFound) {
    11301134        fillContainerFromString(fragment, string);
     1135        return fragment;
     1136    }
     1137
     1138    if (string.length() == 1 && string[0] == '\n') {
     1139        // This is a single newline char, thus just create one HTMLBRElement.
     1140        fragment->appendChild(createHTMLBRElement());
    11311141        return fragment;
    11321142    }
    … …  
    11501160        if (s.isEmpty() && i + 1 == numLines) {
    11511161            // For last line, use the "magic BR" rather than a P.
    1152             element = HTMLBRElement::create(document);
    1153             element->setAttributeWithoutSynchronization(classAttr, AppleInterchangeNewline);
     1162            element = createHTMLBRElement();
    11541163        } else if (useLineBreak) {
    11551164            element = HTMLBRElement::create(document);

  • trunk/Tools/ChangeLog

    r245908 r245912  
     12019-05-30  Andres Gonzalez  <andresg_22@apple.com>
     2
     3        Inserting a newline in contenteditable causes two characters to be added instead of one
     4        https://bugs.webkit.org/show_bug.cgi?id=197894
     5        <rdar://problem/49700998>
     6
     7        Reviewed by Wenson Hsieh and Chris Fleizach.
     8
     9        iOS implementation of several AccessibilityUIElement methods to execute
     10        LayoutTests.
     11 
     12        * WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:
     13        (WTR::AccessibilityUIElement::selectedTextRange):
     14        (WTR::AccessibilityUIElement::setSelectedTextRange):
     15        (WTR::AccessibilityUIElement::replaceTextInRange):
     16
    1172019-05-30  Keith Miller  <keith_miller@apple.com>
    218

  • trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm

    r244059 r245912  
    5656- (CGPoint)accessibilityClickPoint;
    5757- (void)accessibilityModifySelection:(WebCore::TextGranularity)granularity increase:(BOOL)increase;
     58- (NSRange)_accessibilitySelectedTextRange;
     59- (void)_accessibilitySetSelectedTextRange:(NSRange)range;
     60- (BOOL)accessibilityReplaceRange:(NSRange)range withText:(NSString *)string;
    5861- (void)accessibilitySetPostedNotificationCallback:(AXPostedNotificationCallback)function withContext:(void*)context;
    5962- (CGFloat)_accessibilityMinValue;
    … …  
    837840JSRetainPtr<JSStringRef> AccessibilityUIElement::selectedTextRange()
    838841{
    839     return createEmptyJSString();
     842    NSRange range = [m_element _accessibilitySelectedTextRange];
     843    NSMutableString *rangeDescription = [NSMutableString stringWithFormat:@"{%lu, %lu}", static_cast<unsigned long>(range.location), static_cast<unsigned long>(range.length)];
     844    return [rangeDescription createJSStringRef];
    840845}
    841846
    … …  
    847852bool AccessibilityUIElement::setSelectedTextRange(unsigned location, unsigned length)
    848853{
    849     return false;
     854    [m_element _accessibilitySetSelectedTextRange:NSMakeRange(location, length)];
     855    return true;
    850856}
    851857
    … …  
    11461152}
    11471153   
    1148 bool AccessibilityUIElement::replaceTextInRange(JSStringRef, int, int)
    1149 {
    1150     return false;
     1154bool AccessibilityUIElement::replaceTextInRange(JSStringRef string, int location, int length)
     1155{
     1156    return [m_element accessibilityReplaceRange:NSMakeRange(location, length) withText:[NSString stringWithJSStringRef:string]];
    11511157}
    11521158
Note: See TracChangeset for help on using the changeset viewer.