Bug 123163 -   is forced on SPACE between text nodes
Summary:   is forced on SPACE between text nodes
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on: 163962
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-22 07:55 PDT by Frederico Caldeira Knabben
Modified: 2018-04-09 23:16 PDT (History)
12 users (show)

See Also:


Attachments
Test page (1.02 KB, text/html)
2013-10-22 07:55 PDT, Frederico Caldeira Knabben
no flags Details
Patch (13.14 KB, patch)
2016-10-12 16:01 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.37 MB, application/zip)
2016-10-12 16:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.94 MB, application/zip)
2016-10-12 16:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-10-12 17:04 PDT, Build Bot
no flags Details
Patch (22.52 KB, patch)
2016-10-13 23:00 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.68 MB, application/zip)
2016-10-14 00:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (886.31 KB, application/zip)
2016-10-14 00:21 PDT, Build Bot
no flags Details
Patch (23.16 KB, patch)
2016-10-14 00:35 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2016-10-14 00:47 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (24.75 KB, patch)
2016-10-17 14:27 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (25.03 KB, patch)
2016-10-18 18:36 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frederico Caldeira Knabben 2013-10-22 07:55:40 PDT
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.
Comment 1 Frederico Caldeira Knabben 2013-10-22 08:04:36 PDT
Cross post on Blink:
https://code.google.com/p/chromium/issues/detail?id=310149
Comment 2 Frederico Caldeira Knabben 2013-10-22 08:31:37 PDT
Related CKEditor issue: http://dev.ckeditor.com/ticket/9929
Comment 3 Christophe Vidal 2013-12-13 07:37:11 PST
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.
Comment 4 Joone Hur 2016-10-12 16:01:11 PDT
Created attachment 291412 [details]
Patch
Comment 5 Joone Hur 2016-10-12 16:11:19 PDT
This bug was fixed in Blink so it would be good to fix the same bug in WebKit.
https://codereview.chromium.org/2175163004
Comment 6 Build Bot 2016-10-12 16:42:57 PDT
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
Comment 7 Build Bot 2016-10-12 16:43:01 PDT
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 8 Build Bot 2016-10-12 16:54:54 PDT
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
Comment 9 Build Bot 2016-10-12 16:54:58 PDT
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 10 Build Bot 2016-10-12 17:03:59 PDT
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
Comment 11 Build Bot 2016-10-12 17:04:02 PDT
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
Comment 12 Joone Hur 2016-10-13 23:00:27 PDT
Created attachment 291572 [details]
Patch
Comment 13 Build Bot 2016-10-14 00:06:28 PDT
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
Comment 14 Build Bot 2016-10-14 00:06:32 PDT
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 15 Build Bot 2016-10-14 00:21:10 PDT
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
Comment 16 Build Bot 2016-10-14 00:21:15 PDT
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
Comment 17 Joone Hur 2016-10-14 00:35:15 PDT
Created attachment 291584 [details]
Patch
Comment 18 Joone Hur 2016-10-14 00:47:35 PDT
Created attachment 291587 [details]
Patch
Comment 19 Joone Hur 2016-10-14 09:53:54 PDT
I've fixed all the layout test fails so it would be good to review my patch.
Comment 20 Ryosuke Niwa 2016-10-14 14:33:06 PDT
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 21 Darin Adler 2016-10-14 15:20:42 PDT
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 22 Joone Hur 2016-10-17 13:39:37 PDT
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.
Comment 23 Joone Hur 2016-10-17 14:27:11 PDT
Created attachment 291875 [details]
Patch
Comment 24 WebKit Commit Bot 2016-10-17 14:39:29 PDT
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.
Comment 25 Ryosuke Niwa 2016-10-17 15:07:37 PDT
(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?
Comment 26 Joone Hur 2016-10-17 15:52:13 PDT
(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.
Comment 27 Joone Hur 2016-10-17 19:25:09 PDT
Then, when we add another space, a nbsp is added next to the collapsed regular space.
Comment 28 Joone Hur 2016-10-18 16:35:22 PDT
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.
Comment 29 Ryosuke Niwa 2016-10-18 17:20:21 PDT
(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.
Comment 30 Joone Hur 2016-10-18 18:36:48 PDT
Created attachment 292023 [details]
Patch
Comment 31 Joone Hur 2016-10-18 18:37:33 PDT
I've updated the patch.
Comment 32 WebKit Commit Bot 2016-10-19 16:52:27 PDT
Comment on attachment 292023 [details]
Patch

Clearing flags on attachment: 292023

Committed r207578: <http://trac.webkit.org/changeset/207578>
Comment 33 WebKit Commit Bot 2016-10-19 16:52:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Ryosuke Niwa 2016-10-25 12:09:39 PDT
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.
Comment 35 Joone Hur 2016-10-27 22:06:12 PDT
(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.
Comment 36 Ryosuke Niwa 2018-03-19 21:21:22 PDT
(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.
Comment 37 Joone Hur 2018-04-09 23:16:48 PDT
Ok, I will take a look at this bug again.