Created attachment 214858 [details] Test page In contenteditable, if you hit SPACE in-between two successive text nodes, a is being inserted instead of a plain space. The attached test page makes the issue clear. Confirmed on both Safari and Chrome. Works fine with Firefox.
Cross post on Blink: https://code.google.com/p/chromium/issues/detail?id=310149
Related CKEditor issue: http://dev.ckeditor.com/ticket/9929
Confirmed on Chromium 30.0.1599.15 running on Gentoo Linux. It's worth noting that non-breaking spaces also appear when copy-pasting parts of texts: this happens on pasted selection boundaries, when boundaries are spaces. Works fine on Opera Presto.
Created attachment 291412 [details] Patch
This bug was fixed in Blink so it would be good to fix the same bug in WebKit. https://codereview.chromium.org/2175163004
Comment on attachment 291412 [details] Patch Attachment 291412 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2273299 New failing tests: editing/pasteboard/paste-2.html editing/mac/spelling/autocorrection-blockquote-crash.html accessibility/mac/select-text/select-text-9.html accessibility/mac/select-text/select-text-135575.html accessibility/mac/select-text/select-text-7.html editing/pasteboard/unrendered-br.html accessibility/mac/find-and-replace-match-capitalization.html accessibility/mac/select-text/select-text-8.html editing/execCommand/paste-1.html
Created attachment 291417 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291412 [details] Patch Attachment 291412 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2273309 New failing tests: editing/pasteboard/paste-2.html accessibility/mac/find-and-replace-match-capitalization.html accessibility/mac/select-text/select-text-9.html accessibility/mac/select-text/select-text-135575.html accessibility/mac/select-text/select-text-7.html editing/pasteboard/unrendered-br.html editing/mac/spelling/autocorrection-blockquote-crash.html accessibility/mac/select-text/select-text-8.html editing/execCommand/paste-1.html
Created attachment 291420 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291412 [details] Patch Attachment 291412 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2273369 New failing tests: editing/pasteboard/paste-2.html accessibility/mac/select-text/select-text-9.html accessibility/mac/select-text/select-text-135575.html accessibility/mac/select-text/select-text-7.html editing/pasteboard/unrendered-br.html accessibility/mac/find-and-replace-match-capitalization.html accessibility/mac/select-text/select-text-8.html editing/execCommand/paste-1.html
Created attachment 291427 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 291572 [details] Patch
Comment on attachment 291572 [details] Patch Attachment 291572 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2282340 New failing tests: editing/mac/spelling/autocorrection-blockquote-crash.html
Created attachment 291580 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291572 [details] Patch Attachment 291572 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2282462 New failing tests: editing/mac/spelling/autocorrection-blockquote-crash.html
Created attachment 291581 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 291584 [details] Patch
Created attachment 291587 [details] Patch
I've fixed all the layout test fails so it would be good to review my patch.
Comment on attachment 291587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291587&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:904 > + && !(textNode->nextSibling() && textNode->nextSibling()->isTextNode())); What happens when the next text node stars with a whitespace? Is there a code that guarantees that space to be turned into nbsp? > Source/WebCore/editing/htmlediting.cpp:400 > -String stringWithRebalancedWhitespace(const String& string, bool startIsStartOfParagraph, bool endIsEndOfParagraph) > +String stringWithRebalancedWhitespace(const String& string, bool startIsStartOfParagraph, bool shouldEmitNBSPbeforeEnd) We might want to spell out NonBreakingSpace instead of NBSP. > LayoutTests/accessibility/mac/find-and-replace-match-capitalization.html:23 > - shouldBe("document.getElementById('text').innerHTML", "'The Test jumped high.'"); > + shouldBe("document.getElementById('text').innerHTML", "'The Test jumped high.'"); It looks like we still get between "Test" and "jumped" here?
Comment on attachment 291587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291587&action=review >> Source/WebCore/editing/CompositeEditCommand.cpp:904 >> + && !(textNode->nextSibling() && textNode->nextSibling()->isTextNode())); > > What happens when the next text node stars with a whitespace? > Is there a code that guarantees that space to be turned into nbsp? This question is the reason I didn’t review this patch. I don’t understand at all why just knowing the next node is a text node tells us what we need to know!
Comment on attachment 291587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291587&action=review >>> Source/WebCore/editing/CompositeEditCommand.cpp:904 >>> + && !(textNode->nextSibling() && textNode->nextSibling()->isTextNode())); >> >> What happens when the next text node stars with a whitespace? >> Is there a code that guarantees that space to be turned into nbsp? > > This question is the reason I didn’t review this patch. I don’t understand at all why just knowing the next node is a text node tells us what we need to know! 1) The first space is nbsp and the next space is a regular space. It works like Firefox. 2) stringWithRebalancedWhitespace() makes a decision, which space is inserted. 3) When we insert a space between text nodes, A regular space should be inserted instead of nbsp. So we need to know the next node is a text. >> Source/WebCore/editing/htmlediting.cpp:400 >> +String stringWithRebalancedWhitespace(const String& string, bool startIsStartOfParagraph, bool shouldEmitNBSPbeforeEnd) > > We might want to spell out NonBreakingSpace instead of NBSP. I will fix it. >> LayoutTests/accessibility/mac/find-and-replace-match-capitalization.html:23 >> + shouldBe("document.getElementById('text').innerHTML", "'The Test jumped high.'"); > > It looks like we still get between "Test" and "jumped" here? It looks like a separate issue.
Created attachment 291875 [details] Patch
Comment on attachment 291875 [details] Patch Rejecting attachment 291875 [details] from review queue. joone@webkit.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
(In reply to comment #22) > Comment on attachment 291587 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291587&action=review > > >>> Source/WebCore/editing/CompositeEditCommand.cpp:904 > >>> + && !(textNode->nextSibling() && textNode->nextSibling()->isTextNode())); > >> > >> What happens when the next text node stars with a whitespace? > >> Is there a code that guarantees that space to be turned into nbsp? > > > > This question is the reason I didn’t review this patch. I don’t understand at all why just knowing the next node is a text node tells us what we need to know! > > 1) The first space is nbsp and the next space is a regular space. It works > like Firefox. > 2) stringWithRebalancedWhitespace() makes a decision, which space is > inserted. > 3) When we insert a space between text nodes, A regular space should be > inserted instead of nbsp. So we need to know the next node is a text. This doesn't answer my question. When the first text node ends with a regular space and the next text node starts with a regular space, then those two spaces would be collapsed into a single space. What prevents this from happening?
(In reply to comment #25) > (In reply to comment #22) > > Comment on attachment 291587 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=291587&action=review > > > > >>> Source/WebCore/editing/CompositeEditCommand.cpp:904 > > >>> + && !(textNode->nextSibling() && textNode->nextSibling()->isTextNode())); > > >> > > >> What happens when the next text node stars with a whitespace? > > >> Is there a code that guarantees that space to be turned into nbsp? > > > > > > This question is the reason I didn’t review this patch. I don’t understand at all why just knowing the next node is a text node tells us what we need to know! > > > > 1) The first space is nbsp and the next space is a regular space. It works > > like Firefox. > > 2) stringWithRebalancedWhitespace() makes a decision, which space is > > inserted. > > 3) When we insert a space between text nodes, A regular space should be > > inserted instead of nbsp. So we need to know the next node is a text. > > This doesn't answer my question. > > When the first text node ends with a regular space and the next text node > starts with a regular space, then those two spaces would be collapsed into a > single space. What prevents this from happening? Yes, two spaces are collapsed into a single space. Anything doesn't prevent this. Then, we add another space, nbsp is added.
Then, when we add another space, a nbsp is added next to the collapsed regular space.
My understanding is that you wanted me to fix the problem of collapsing two regular spaces into a regular space for the case( text(space) + (space)text ). => this is a layout issue. With checking the first character of the next text, when we add an additional space between two texts in this case, there are two cases we can do this: (1) when we add a space at the end of the first text, three spaces are created as follows: A B (2) When we add a space at the start of the second text, nbsp is added as follows: A B My question is that do we need to fix the first layout issue? If not, 2) is right. If so, (1) is right.
(In reply to comment #28) > My understanding is that you wanted me to fix the problem of collapsing two > regular spaces into a regular space for the case( text(space) + (space)text > ). => this is a layout issue. > > With checking the first character of the next text, when we add an > additional space between two texts in this case, there are two cases we can > do this: > (1) when we add a space at the end of the first text, three spaces are > created as follows: A B This is correct behavior. If we don't have that nbsp, all three spaces would be collapsed into one. > (2) When we add a space at the start of the second text, nbsp is added as > follows: > A B This is also correct behavior. Without nbsp, two spaces would be collapsed.
Created attachment 292023 [details] Patch
I've updated the patch.
Comment on attachment 292023 [details] Patch Clearing flags on attachment: 292023 Committed r207578: <http://trac.webkit.org/changeset/207578>
All reviewed patches have been landed. Closing bug.
This patch broke https://quip.com. User can no longer type a space on that app. We should probably roll out this pitch given the severity of the bug.
(In reply to comment #34) > This patch broke https://quip.com. User can no longer type a space on that > app. We should probably roll out this pitch given the severity of the bug. How can I reproduce the bug? I could type a space in the editor.
(In reply to Joone Hur from comment #35) > (In reply to comment #34) > > This patch broke https://quip.com. User can no longer type a space on that > > app. We should probably roll out this pitch given the severity of the bug. > > How can I reproduce the bug? I could type a space in the editor. Reproduction steps: 1. Apply your patch to WebKit 2. Copy & paste a link 3. Press the space bar. The space isn't inserted.
Ok, I will take a look at this bug again.
I am able to reproduce this bug in Safari 15.6 on macOS 12.5 using attached test case and it fails on "Test 1", where adding "Space" between A & B, gives following output: <p id="p_1" contenteditable="true">Test 1: Type a SPACE between A and B: A B</p> While all other browsers (Chrome Canary 106 and Firefox Nightly 104) does not add but add " " (literal space) in between text similar to Test 2. Just wanted to share updated testing results. Thanks!
<rdar://problem/97621106>
Committed 257136@main (54804ca28d80): <https://commits.webkit.org/257136@main> Reviewed commits have been landed. Closing PR #6639 and removing active labels.