Bug 123163 –   is forced on SPACE between text nodes

• Component: HTML Editing
• Status: RESOLVED FIXED

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

[Christophe Vidal]

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.

[Joone Hur]

Created attachment 291412 [details]
Patch

[Joone Hur]

This bug was fixed in Blink so it would be good to fix the same bug in WebKit.
https://codereview.chromium.org/2175163004

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joone Hur]

Created attachment 291572 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joone Hur]

Created attachment 291584 [details]
Patch

[Joone Hur]

Created attachment 291587 [details]
Patch

[Joone Hur]

I've fixed all the layout test fails so it would be good to review my patch.

[Ryosuke Niwa]

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?

[Darin Adler]

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!

[Joone Hur]

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.

[Joone Hur]

Created attachment 291875 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 291875 [details]
Patch

Rejecting attachment 291875 [details] from review queue.

[e-mail] 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.

[Ryosuke Niwa]

(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?

[Joone Hur]

(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.

[Joone Hur]

Then, when we add another space, a nbsp is added next to the collapsed regular space.

[Joone Hur]

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.

[Ryosuke Niwa]

(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.

[Joone Hur]

Created attachment 292023 [details]
Patch

[Joone Hur]

I've updated the patch.

[WebKit Commit Bot]

Comment on attachment 292023 [details]
Patch

Clearing flags on attachment: 292023

Committed r207578: <http://trac.webkit.org/changeset/207578>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Joone Hur]

(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.

[Ryosuke Niwa]

(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.

[Joone Hur]

Ok, I will take a look at this bug again.

[Ahmad Saleem]

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&nbsp;B</p>

While all other browsers (Chrome Canary 106 and Firefox Nightly 104) does not add &nbsp; but add " " (literal space) in between text similar to Test 2. Just wanted to share updated testing results. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97621106>

[EWS]

Committed 257136@main (54804ca28d80): <https://commits.webkit.org/257136@main>

Reviewed commits have been landed. Closing PR #6639 and removing active labels.