Inserting a newline in contenteditable causes 2two chars to be added instead of one.
Created attachment 369892 [details] Patch
<rdar://problem/49700998>
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 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 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 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
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
Created attachment 370014 [details] Patch
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.
(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.
(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 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
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 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
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 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
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
Created attachment 370116 [details] Patch
(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
(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 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
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 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
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
Created attachment 370419 [details] Patch
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 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
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
Created attachment 370575 [details] Patch
Only new change in this patch is in test expectations to skip new test on win.
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.
Created attachment 370964 [details] Patch
(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 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.
Created attachment 370981 [details] Patch
(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 on attachment 370981 [details] Patch Clearing flags on attachment: 370981 Committed r245912: <https://trac.webkit.org/changeset/245912>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 371007 [details] Patch
Created attachment 371071 [details] Patch
Comment on attachment 371071 [details] Patch Clearing flags on attachment: 371071 Committed r245980: <https://trac.webkit.org/changeset/245980>
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.
(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*.