Bug 197894 - Inserting a newline in contenteditable causes two characters to be added instead of one
Summary: Inserting a newline in contenteditable causes two characters to be added inst...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-14 14:10 PDT by Andres Gonzalez
Modified: 2019-06-19 23:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.35 KB, patch)
2019-05-14 14:24 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.60 MB, application/zip)
2019-05-14 16:19 PDT, Build Bot
no flags Details
Patch (15.71 KB, patch)
2019-05-15 18:16 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.96 MB, application/zip)
2019-05-15 20:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews211 for win-future (13.45 MB, application/zip)
2019-05-16 00:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews212 for win-future (13.42 MB, application/zip)
2019-05-16 02:15 PDT, Build Bot
no flags Details
Patch (19.67 KB, patch)
2019-05-17 07:11 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.97 MB, application/zip)
2019-05-17 08:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews215 for win-future (13.30 MB, application/zip)
2019-05-17 09:06 PDT, Build Bot
no flags Details
Patch (19.37 KB, patch)
2019-05-22 10:49 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.78 MB, application/zip)
2019-05-22 13:07 PDT, Build Bot
no flags Details
Patch (20.00 KB, patch)
2019-05-24 09:27 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (20.65 KB, patch)
2019-05-30 12:13 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (20.46 KB, patch)
2019-05-30 15:05 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (20.44 KB, patch)
2019-05-30 18:02 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2019-05-31 11:03 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2019-05-14 14:10:18 PDT
Inserting a newline in contenteditable causes 2two chars to be added instead of one.
Comment 1 Andres Gonzalez 2019-05-14 14:24:22 PDT
Created attachment 369892 [details]
Patch
Comment 2 Andres Gonzalez 2019-05-14 14:24:27 PDT
<rdar://problem/49700998>
Comment 3 Build Bot 2019-05-14 14:26:06 PDT
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.
Comment 4 chris fleizach 2019-05-14 14:56:29 PDT
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?
Comment 5 chris fleizach 2019-05-14 15:01:35 PDT
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
Comment 6 Build Bot 2019-05-14 16:19:11 PDT
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
Comment 7 Build Bot 2019-05-14 16:19:20 PDT
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
Comment 8 Andres Gonzalez 2019-05-15 18:16:30 PDT
Created attachment 370014 [details]
Patch
Comment 9 Build Bot 2019-05-15 18:18:04 PDT
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.
Comment 10 Andres Gonzalez 2019-05-15 18:25:47 PDT
(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.
Comment 11 Andres Gonzalez 2019-05-15 18:28:15 PDT
(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.
Comment 12 Build Bot 2019-05-15 20:12:44 PDT
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
Comment 13 Build Bot 2019-05-15 20:12:46 PDT
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
Comment 14 Build Bot 2019-05-16 00:53:25 PDT
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
Comment 15 Build Bot 2019-05-16 00:53:28 PDT
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
Comment 16 Build Bot 2019-05-16 02:15:01 PDT
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
Comment 17 Build Bot 2019-05-16 02:15:03 PDT
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
Comment 18 Andres Gonzalez 2019-05-17 07:11:04 PDT
Created attachment 370116 [details]
Patch
Comment 19 Andres Gonzalez 2019-05-17 07:15:12 PDT
(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
Comment 20 Andres Gonzalez 2019-05-17 07:25:52 PDT
(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.
Comment 21 Build Bot 2019-05-17 08:59:37 PDT
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
Comment 22 Build Bot 2019-05-17 08:59:39 PDT
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
Comment 23 Build Bot 2019-05-17 09:06:52 PDT
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
Comment 24 Build Bot 2019-05-17 09:06:54 PDT
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
Comment 25 Andres Gonzalez 2019-05-22 10:49:21 PDT
Created attachment 370419 [details]
Patch
Comment 26 Andres Gonzalez 2019-05-22 11:00:44 PDT
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.
Comment 27 Build Bot 2019-05-22 13:06:51 PDT
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
Comment 28 Build Bot 2019-05-22 13:07:16 PDT
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
Comment 29 Andres Gonzalez 2019-05-24 09:27:01 PDT
Created attachment 370575 [details]
Patch
Comment 30 Andres Gonzalez 2019-05-24 09:30:11 PDT
Only new change in this patch is in test expectations to skip new test on win.
Comment 31 Wenson Hsieh 2019-05-30 10:26:18 PDT
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.
Comment 32 Andres Gonzalez 2019-05-30 12:13:32 PDT
Created attachment 370964 [details]
Patch
Comment 33 Andres Gonzalez 2019-05-30 12:20:32 PDT
(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.
Comment 34 Wenson Hsieh 2019-05-30 12:30:53 PDT
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.
Comment 35 Andres Gonzalez 2019-05-30 15:05:41 PDT
Created attachment 370981 [details]
Patch
Comment 36 Andres Gonzalez 2019-05-30 15:10:32 PDT
(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!
Comment 37 WebKit Commit Bot 2019-05-30 17:03:05 PDT
Comment on attachment 370981 [details]
Patch

Clearing flags on attachment: 370981

Committed r245912: <https://trac.webkit.org/changeset/245912>
Comment 38 WebKit Commit Bot 2019-05-30 17:03:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Andres Gonzalez 2019-05-30 18:02:39 PDT
Reopening to attach new patch.
Comment 40 Andres Gonzalez 2019-05-30 18:02:41 PDT
Created attachment 371007 [details]
Patch
Comment 41 Andres Gonzalez 2019-05-31 11:03:39 PDT
Created attachment 371071 [details]
Patch
Comment 42 WebKit Commit Bot 2019-05-31 13:23:38 PDT
Comment on attachment 371071 [details]
Patch

Clearing flags on attachment: 371071

Committed r245980: <https://trac.webkit.org/changeset/245980>
Comment 43 WebKit Commit Bot 2019-05-31 13:23:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Ryosuke Niwa 2019-06-19 23:40:16 PDT
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.
Comment 45 Ryosuke Niwa 2019-06-19 23:40:43 PDT
(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*.