Bug 197894 – Inserting a newline in contenteditable causes two characters to be added instead of one

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andres Gonzalez]

Inserting a newline in contenteditable causes 2two chars to be added instead of one.

[Andres Gonzalez]

Created attachment 369892 [details]
Patch

[Andres Gonzalez]

<rdar://problem/49700998>

[EWS Watchlist]

Attachment 369892 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 369892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369892&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-2746
> -                return [NSValue valueWithRange:NSMakeRange(0, 0)];

what issue was this one causing?

[chris fleizach]

Comment on attachment 369892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369892&action=review

> Source/WebCore/editing/markup.cpp:1139
> +    }

it looks like this is pretty identical to line 1119.  maybe we can make this HTMLBRElement creation and fragment returning into a closure

[EWS Watchlist]

Comment on attachment 369892 [details]
Patch

Attachment 369892 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12191933

New failing tests:
js/error-should-not-strong-reference-global-object.html

[EWS Watchlist]

Created attachment 369907 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Andres Gonzalez]

Created attachment 370014 [details]
Patch

[EWS Watchlist]

Attachment 370014 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/TextIterator.cpp:1555:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 369892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369892&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-2746
> > -                return [NSValue valueWithRange:NSMakeRange(0, 0)];
> 
> what issue was this one causing?

It is harmless but unnecessary to check for isNull because it will return the same thing, the range is null if and only if both location and length are 0 so this return and the one below will be the same. Thought I should clean up since looking at this code.

[Andres Gonzalez]

(In reply to chris fleizach from comment #5)
> Comment on attachment 369892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369892&action=review
> 
> > Source/WebCore/editing/markup.cpp:1139
> > +    }
> 
> it looks like this is pretty identical to line 1119.  maybe we can make this
> HTMLBRElement creation and fragment returning into a closure

Done. Lambda that creates the HTMLBRElement is reused throughout the function.

[EWS Watchlist]

Comment on attachment 370014 [details]
Patch

Attachment 370014 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12203310

New failing tests:
editing/execCommand/format-block-at-root.html

[EWS Watchlist]

Created attachment 370023 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 370014 [details]
Patch

Attachment 370014 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12204825

New failing tests:
accessibility/set-selected-text-range-after-newline.html
svg/text/textpath-reference-update.html
storage/indexeddb/index-cursor.html
webanimations/accelerated-animation-with-delay-and-seek.html

[EWS Watchlist]

Created attachment 370028 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 370014 [details]
Patch

Attachment 370014 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12205195

New failing tests:
accessibility/set-selected-text-range-after-newline.html

[EWS Watchlist]

Created attachment 370032 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Andres Gonzalez]

Created attachment 370116 [details]
Patch

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #18)
> Created attachment 370116 [details]
> Patch

- Added iOS accessibility test.
- Corrected style issue: m_runLength == 0 -> !m_runLength

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #18)
> Created attachment 370116 [details]
> Patch

creatFragmentFromText, in markup.cpp, was splitting the string “\n” into two strings, one empty, and thus creating two elements to represent the line break, one empty <div> and one <br>. This resulted in the emission of an additional “\n” corresponding to the empty <div> during text extraction (TextIterator.cpp line 1152). The change in CharacterIterator::range is to properly handle the case where a text run consists of a <br> element. Before It was treated as CharacterData and not as a node range.

[EWS Watchlist]

Comment on attachment 370116 [details]
Patch

Attachment 370116 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12215303

New failing tests:
editing/execCommand/format-block-at-root.html

[EWS Watchlist]

Created attachment 370120 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 370116 [details]
Patch

Attachment 370116 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12215450

New failing tests:
accessibility/set-selected-text-range-after-newline.html

[EWS Watchlist]

Created attachment 370121 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Andres Gonzalez]

Created attachment 370419 [details]
Patch

[Andres Gonzalez]

In latest patch instead of implementing a workaround for CharacterIterator::range, used a workaround for visiblePositionForIndexUsingCharacterIterator, similar to nextBoundary in VisibleUnits.cpp line 688 (see the comment concerning rdar://5192593. This fixes the problem of setting the selection to the character following a line break and keep all editing tests passing.

[EWS Watchlist]

Comment on attachment 370419 [details]
Patch

Attachment 370419 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12260380

New failing tests:
accessibility/set-selected-text-range-after-newline.html

[EWS Watchlist]

Created attachment 370435 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Andres Gonzalez]

Created attachment 370575 [details]
Patch

[Andres Gonzalez]

Only new change in this patch is in test expectations to skip new test on win.

[Wenson Hsieh]

Comment on attachment 370575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370575&action=review

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

You should add a ChangeLog entry here describing the bug and the fix.

> Source/WebCore/editing/Editing.cpp:1127
> +        RefPtr<Range> r = it.range();

Nits:
- You can use auto here instead of RefPtr<Range>.
- We also generally avoid single character variable names like `r`, in favor of `range`.

> LayoutTests/accessibility/set-selected-text-range-after-newline.html:21
> +                          text.replaceTextInRange("\n", 5, 0);

Nit - indentation looks off here. We don't align to the start of the brace/parenthesis like this in WebKit.

[Andres Gonzalez]

Created attachment 370964 [details]
Patch

[Andres Gonzalez]

(In reply to Wenson Hsieh from comment #31)
> Comment on attachment 370575 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370575&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Reviewed by NOBODY (OOPS!).
> 
> You should add a ChangeLog entry here describing the bug and the fix.
>
Done.
 
> > Source/WebCore/editing/Editing.cpp:1127
> > +        RefPtr<Range> r = it.range();
> 
> Nits:
> - You can use auto here instead of RefPtr<Range>.
> - We also generally avoid single character variable names like `r`, in favor
> of `range`.
>
Done.
 
> > LayoutTests/accessibility/set-selected-text-range-after-newline.html:21
> > +                          text.replaceTextInRange("\n", 5, 0);
> 
> Nit - indentation looks off here. We don't align to the start of the
> brace/parenthesis like this in WebKit.

Fixed. Was using Xcode default indentation.

[Wenson Hsieh]

Comment on attachment 370964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370964&action=review

Seems reasonable, though I'm not familiar with the accessibility part of this patch.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1624
> +        Node* node = this->node();

Nit - auto

> LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html:35
> +        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();
> +                                            });
> +                          });

Nit - indentation still looks off.

[Andres Gonzalez]

Created attachment 370981 [details]
Patch

[Andres Gonzalez]

(In reply to Wenson Hsieh from comment #34)
> Comment on attachment 370964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370964&action=review
> 
> Seems reasonable, though I'm not familiar with the accessibility part of
> this patch.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1624
> > +        Node* node = this->node();
> 
> Nit - auto

Done.
 
> > LayoutTests/accessibility/ios-simulator/set-selected-text-range-after-newline.html:35
> > +        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();
> > +                                            });
> > +                          });
> 
> Nit - indentation still looks off.


Fixed now for iOS test. Thanks!

[WebKit Commit Bot]

Comment on attachment 370981 [details]
Patch

Clearing flags on attachment: 370981

Committed r245912: <https://trac.webkit.org/changeset/245912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andres Gonzalez]

Reopening to attach new patch.

[Andres Gonzalez]

Created attachment 371007 [details]
Patch

[Andres Gonzalez]

Created attachment 371071 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 371071 [details]
Patch

Clearing flags on attachment: 371071

Committed r245980: <https://trac.webkit.org/changeset/245980>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 371007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371007&action=review

> Source/WebCore/editing/Editing.cpp:1131
> +        if (range->startPosition() == range->endPosition()) {
> +            it.advance(1);
> +            return VisiblePosition(it.range()->startPosition());
> +        }

This code is correct. it.advance(1) can put the text iterator at the end, and if so, calling it.range() would result in a crash.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #44)
> Comment on attachment 371007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371007&action=review
> 
> > Source/WebCore/editing/Editing.cpp:1131
> > +        if (range->startPosition() == range->endPosition()) {
> > +            it.advance(1);
> > +            return VisiblePosition(it.range()->startPosition());
> > +        }
> 
> This code is correct. it.advance(1) can put the text iterator at the end,
> and if so, calling it.range() would result in a crash.

This code is *incorrect*.