Unable to delete the first and the last characters of a contenteditable div with display: table.

Unable to delete the first and the last characters of a contenteditable div with display: table.

Contenteditable cannot be set for table elements. So throughout our editing code we have checks wherein if the node being processed is a table node, we check for contenteditability on its parent.

The current check: isTableElement() however checks based on the renderer's style display type. If the display of the renderer was set to TABLE or INLINE_TABLE, it would be considered as a node to be skipped (for editing purposes) and contenteditability verified against its parent.

However, with this approach, block elements whose display type has been set to TABLE and who also have contenteditable set for them, too would get skipped; since the check was being carried out against the renderer and not the element type.

Methods such as isEditablePosition(), isCandidate(), upstream(), downstream() etc., basically all methods that are concerned with either checking the validity of the current position or dealing with position traversal should thus check against the element's name (table) instead of the renderer.

The isTableElement() function is used only for editing purposes. Have thus modified the same to only check against the tag name.
Another method isRenderedTable() has been added which performs the old check (against the renderer's display type).

One existing test's expected result changes after this fix. The test is a crashtest and an extra space character gets added now. This is because for that test, display: table set on the body element is now being respected and when formatted, an extra space is appended at the end. The extra space gets added for formatting carried out on the body element even with the original code.
More explanation with screenshots for this change have been added to the corresponding bug: #107366

BUG=107366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162687

[yosin_UTC9, 2013-11-06 01:29:33]

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
File LayoutTests/editing/execCommand/format-block-at-root-expected.txt (right):

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/format-block-at-root-expected.txt:3: if
(window.testRunner) testRunner.dumpAsText(); document.designMode = "on"; range =
document.createRange(); range.selectNodeContents(document.body);
window.getSelection().addRange(range); document.execCommand("FormatBlock",
false, "h1");
Just curiosity, what characters is added, U+0020, U+000D or another?

https://codereview.chromium.org/59963002/diff/1/Source/core/editing/htmlediti...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/59963002/diff/1/Source/core/editing/htmlediti...
Source/core/editing/htmlediting.cpp:755: bool isTableElement(Node* n)
To reduce the risk of changing this function. How about renaming this function
to isRenderedTable(), to follow FIXME, then introduce a new isTableElement()?

Testing coverage may not be 100%. It is better to limit scope of changes.

[arpitab_, 2013-11-06 06:59:51]

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
File LayoutTests/editing/execCommand/format-block-at-root-expected.txt (right):

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/format-block-at-root-expected.txt:3: if
(window.testRunner) testRunner.dumpAsText(); document.designMode = "on"; range =
document.createRange(); range.selectNodeContents(document.body);
window.getSelection().addRange(range); document.execCommand("FormatBlock",
false, "h1");
On 2013/11/06 01:29:34, Yoshi wrote:
> Just curiosity, what characters is added, U+0020, U+000D or another?
That's a non-breaking space, U+00A0.

https://codereview.chromium.org/59963002/diff/1/Source/core/editing/htmlediti...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/59963002/diff/1/Source/core/editing/htmlediti...
Source/core/editing/htmlediting.cpp:755: bool isTableElement(Node* n)
On 2013/11/06 01:29:34, Yoshi wrote:
> To reduce the risk of changing this function. How about renaming this function
> to isRenderedTable(), to follow FIXME, then introduce a new isTableElement()?
> 
> Testing coverage may not be 100%. It is better to limit scope of changes.
I understand. I'll add a new isTableElement().

[yosin_UTC9, 2013-11-06 07:32:39]

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
File LayoutTests/editing/execCommand/format-block-at-root-expected.txt (right):

https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
LayoutTests/editing/execCommand/format-block-at-root-expected.txt:3: if
(window.testRunner) testRunner.dumpAsText(); document.designMode = "on"; range =
document.createRange(); range.selectNodeContents(document.body);
window.getSelection().addRange(range); document.execCommand("FormatBlock",
false, "h1");
On 2013/11/06 06:59:51, arpitab_ wrote:
> On 2013/11/06 01:29:34, Yoshi wrote:
> > Just curiosity, what characters is added, U+0020, U+000D or another?
> That's a non-breaking space, U+00A0.

I see. I hope these   will be removed once we have isRenderedTable and
isTableElement. We don't need to have these  

[arpitab_, 2013-11-07 10:08:49]

On 2013/11/06 07:32:39, Yoshi wrote:
>
https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
> File LayoutTests/editing/execCommand/format-block-at-root-expected.txt
(right):
> 
>
https://codereview.chromium.org/59963002/diff/1/LayoutTests/editing/execComma...
> LayoutTests/editing/execCommand/format-block-at-root-expected.txt:3: if
> (window.testRunner) testRunner.dumpAsText(); document.designMode = "on"; range
=
> document.createRange(); range.selectNodeContents(document.body);
> window.getSelection().addRange(range); document.execCommand("FormatBlock",
> false, "h1");
> On 2013/11/06 06:59:51, arpitab_ wrote:
> > On 2013/11/06 01:29:34, Yoshi wrote:
> > > Just curiosity, what characters is added, U+0020, U+000D or another?
> > That's a non-breaking space, U+00A0.
> 
> I see. I hope these   will be removed once we have isRenderedTable and
> isTableElement. We don't need to have these  

Hi yosin,
Actually this extra space seems to be getting added if contenteditable is set on
the <body> and then "FormatBlock" command is executed on it.

This same behavior (extra space) is observed on other browsers such as FF as
well.

I created a similar testcase (as the failing test) and have taken screenshots
for the new (post fix) and old browser behavior for Chrome and for the behavior
on FF as well.

Am attaching these to the corresponding crbug:
http://code.google.com/p/chromium/issues/detail?id=107366.

I will upload another patch having two methods (as you had mentioned earlier)
soon.

[arpitab_, 2013-11-07 11:15:35]

Hi Yoshi,

Have uploaded another patch with two separate methods: isTableElement() - that
checks against the tag's name and isRenderedTable() - that verifies against the
node's renderer.

However, since isTableElement() is used only for the editing code and, I
believe, used only where we need to have special handling since contenteditable
cannot be specified on the table element, we can continue using only one method:
isTableElement instead of having two. Would appreciate your thoughts on the
same.

[yosin_UTC9, 2013-11-08 01:44:11]

On 2013/11/07 11:15:35, arpitab_ wrote:
> Hi Yoshi,
> 
> Have uploaded another patch with two separate methods: isTableElement() - that
> checks against the tag's name and isRenderedTable() - that verifies against
the
> node's renderer.
> 
> However, since isTableElement() is used only for the editing code and, I
> believe, used only where we need to have special handling since
contenteditable
> cannot be specified on the table element, we can continue using only one
method:
> isTableElement instead of having two. Would appreciate your thoughts on the
> same.

Please split this patch into two, one for rename isTableElement() to
isRenderedTable() as FIXME, another one introduces isTableElement() and changes
call sites, e.g. changes in Caret.cpp doesn't relate to issue of deleting
characters.

I would like to change Blink code incrementally. Once, we get another issue
related to isRenderedTable(), we can change to use isTableElement() that time.

[yosin_UTC9, 2013-11-08 01:44:45]

https://codereview.chromium.org/59963002/diff/100001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/format-block-at-root-expected.txt (right):

https://codereview.chromium.org/59963002/diff/100001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/format-block-at-root-expected.txt:2: Test passes
if it does not crash.
which function insert extra &nbsp to format-block-at-root.html?

[arpitab_, 2013-11-08 12:39:53]

On 2013/11/08 01:44:11, Yoshi wrote:
> On 2013/11/07 11:15:35, arpitab_ wrote:
> > Hi Yoshi,
> > 
> > Have uploaded another patch with two separate methods: isTableElement() -
that
> > checks against the tag's name and isRenderedTable() - that verifies against
> the
> > node's renderer.
> > 
> > However, since isTableElement() is used only for the editing code and, I
> > believe, used only where we need to have special handling since
> contenteditable
> > cannot be specified on the table element, we can continue using only one
> method:
> > isTableElement instead of having two. Would appreciate your thoughts on the
> > same.
> 
> Please split this patch into two, one for rename isTableElement() to
> isRenderedTable() as FIXME, another one introduces isTableElement() and
changes
> call sites, e.g. changes in Caret.cpp doesn't relate to issue of deleting
> characters.
> 
> I would like to change Blink code incrementally. Once, we get another issue
> related to isRenderedTable(), we can change to use isTableElement() that time.

Hi Yoshi,

Thanks for the review.

Have submitted a patch for renaming isTableElement() as suggested.
https://codereview.chromium.org/66383003/

I'll follow that up with the addition of the new method for fixing this issue.

[arpitab_, 2013-11-14 06:07:02]

Hi Yoshi,

As discussed, have now added another patch here that introduces the
isTableElement() method for fixing this issue.

Please review the same.

[yosin_UTC9, 2013-11-14 06:57:59]

Do we really need to replace these isRenderedTable() to isTableElement() for
deleting first/last characters in display:table? It seems changes of
Position.cpp is enough.

https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table.html (right):

https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table.html:22:
document.execCommand("forwardDelete");
I think js-test.js is easier to write test and verify result.

function testIt(caretPosition, deleteCommand, expectedString)
{
    testDiv.innerText = 'ABCD';
    selection.collapse(testDiv, selectionPosition);
    document.execCommand(deleteCommand);
    shouldBeEqualToString('testDiv.textContent', expectedString);
}

testIt(0, 'Delete', 'BCD');
testIt(1, 'forwardDelete', 'ABC');
finishJSTest();

Note: It is better to make each test case independent, e.g. not use modified
sample in following test cases.

https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:980: lastNode =
outerNode->cloneNode(isTableElement(outerNode.get()));
I don't think this changes related to deleting characters.

https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
Source/core/editing/CompositeEditCommand.cpp:995: RefPtr<Node> child =
item->cloneNode(isTableElement(item));
I don't think this changes related to deleting characters.

[arpitab_, 2013-11-14 07:05:38]

On 2013/11/14 06:57:59, Yoshi wrote:
> Do we really need to replace these isRenderedTable() to isTableElement() for
> deleting first/last characters in display:table? It seems changes of
> Position.cpp is enough.
> 
>
https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
> File LayoutTests/editing/deleting/display-table.html (right):
> 
>
https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
> LayoutTests/editing/deleting/display-table.html:22:
> document.execCommand("forwardDelete");
> I think js-test.js is easier to write test and verify result.
> 
> function testIt(caretPosition, deleteCommand, expectedString)
> {
>     testDiv.innerText = 'ABCD';
>     selection.collapse(testDiv, selectionPosition);
>     document.execCommand(deleteCommand);
>     shouldBeEqualToString('testDiv.textContent', expectedString);
> }
> 
> testIt(0, 'Delete', 'BCD');
> testIt(1, 'forwardDelete', 'ABC');
> finishJSTest();
> 
> Note: It is better to make each test case independent, e.g. not use modified
> sample in following test cases.
> 
>
https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
> File Source/core/editing/CompositeEditCommand.cpp (right):
> 
>
https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
> Source/core/editing/CompositeEditCommand.cpp:980: lastNode =
> outerNode->cloneNode(isTableElement(outerNode.get()));
> I don't think this changes related to deleting characters.
> 
>
https://codereview.chromium.org/59963002/diff/190001/Source/core/editing/Comp...
> Source/core/editing/CompositeEditCommand.cpp:995: RefPtr<Node> child =
> item->cloneNode(isTableElement(item));
> I don't think this changes related to deleting characters.

This change is required. cloneNode() takes a bool "deep" as its in param
depending on which it either clones the specified node including its children
(in case of a table element) or not (in case of a div having display: table).
return deep ? cloneElementWithChildren() : cloneElementWithoutChildren();
If we don't do this, existing test will fail.

[arpitab_, 2013-11-14 07:08:14]

https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table.html (right):

https://codereview.chromium.org/59963002/diff/190001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table.html:22:
document.execCommand("forwardDelete");
On 2013/11/14 06:57:59, Yoshi wrote:
> I think js-test.js is easier to write test and verify result.
> 
> function testIt(caretPosition, deleteCommand, expectedString)
> {
>     testDiv.innerText = 'ABCD';
>     selection.collapse(testDiv, selectionPosition);
>     document.execCommand(deleteCommand);
>     shouldBeEqualToString('testDiv.textContent', expectedString);
> }
> 
> testIt(0, 'Delete', 'BCD');
> testIt(1, 'forwardDelete', 'ABC');
> finishJSTest();
> 
> Note: It is better to make each test case independent, e.g. not use modified
> sample in following test cases.

Understand. I will post another patch with the mentioned changes.

[arpitab_, 2013-11-14 10:19:50]

Hi Yoshi,

Have posted another patch with the modified testcase using js-test.js instead of
the markup test.

[yosin_UTC9, 2013-11-15 01:57:49]

ACK

https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table-expected.txt (right):

https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table-expected.txt:1: Testcase for bug
http://crbug.com/107366: Can't delete first and last characters of a root
editable element with display: table
nit: We don't need to have description and other text for human.
I usually remove them for reducing size of expected text file.

<div id="container">...</div>
<div id="log"></div> <!-- "PASS" "TEST COMPLETE" go here -->

<script>
...
if (window.testRunner)
  document.getElementById('container').outerHTML = '';
finishJSTest();
</script>

[arpitab_, 2013-11-15 06:14:24]

https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table-expected.txt (right):

https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table-expected.txt:1: Testcase for bug
http://crbug.com/107366: Can't delete first and last characters of a root
editable element with display: table
On 2013/11/15 01:57:50, Yoshi wrote:
> nit: We don't need to have description and other text for human.
> I usually remove them for reducing size of expected text file.
> 
> <div id="container">...</div>
> <div id="log"></div> <!-- "PASS" "TEST COMPLETE" go here -->
> 
> <script>
> ...
> if (window.testRunner)
>   document.getElementById('container').outerHTML = '';
> finishJSTest();
> </script>

Done.

[arpitab_, 2013-11-15 06:17:39]

On 2013/11/15 01:57:49, Yoshi wrote:
> ACK
> 
>
https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
> File LayoutTests/editing/deleting/display-table-expected.txt (right):
> 
>
https://codereview.chromium.org/59963002/diff/280001/LayoutTests/editing/dele...
> LayoutTests/editing/deleting/display-table-expected.txt:1: Testcase for bug
> http://crbug.com/107366: Can't delete first and last characters of a root
> editable element with display: table
> nit: We don't need to have description and other text for human.
> I usually remove them for reducing size of expected text file.
> 
> <div id="container">...</div>
> <div id="log"></div> <!-- "PASS" "TEST COMPLETE" go here -->
> 
> <script>
> ...
> if (window.testRunner)
>   document.getElementById('container').outerHTML = '';
> finishJSTest();
> </script>

Thanks for the review Yoshi! Have posted another patch with the suggested
changes.

[yosin_UTC9, 2013-11-15 06:36:07]

ACK
Thanks!

[leviw_travelin_and_unemployed, 2013-11-26 01:39:43]

LGTM.

https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table.html (right):

https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table.html:15: function
testIt(caretPosition, deleteCommand, expectedString)
Nit: runTest? Also a description of the test would be nice :)

https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
Source/core/editing/htmlediting.cpp:755: bool isTableElement(const Node* n)
Nit: node instead of n

[arpitab_, 2013-11-26 09:08:58]

On 2013/11/26 01:39:43, Levi wrote:
> LGTM.
> 
>
https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
> File LayoutTests/editing/deleting/display-table.html (right):
> 
>
https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
> LayoutTests/editing/deleting/display-table.html:15: function
> testIt(caretPosition, deleteCommand, expectedString)
> Nit: runTest? Also a description of the test would be nice :)
> 
>
https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
> File Source/core/editing/htmlediting.cpp (right):
> 
>
https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
> Source/core/editing/htmlediting.cpp:755: bool isTableElement(const Node* n)
> Nit: node instead of n

Many thanks for the lgtm Levi!

Have posted another patch incorporating the changes you've mentioned.

[arpitab_, 2013-11-26 09:09:08]

https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/display-table.html (right):

https://codereview.chromium.org/59963002/diff/340001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/display-table.html:15: function
testIt(caretPosition, deleteCommand, expectedString)
On 2013/11/26 01:39:43, Levi wrote:
> Nit: runTest? Also a description of the test would be nice :)

Done.

https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/59963002/diff/340001/Source/core/editing/html...
Source/core/editing/htmlediting.cpp:755: bool isTableElement(const Node* n)
On 2013/11/26 01:39:43, Levi wrote:
> Nit: node instead of n

Done.

[commit-bot: I haz the power, 2013-11-26 10:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/59963002/370001

[commit-bot: I haz the power, 2013-11-26 11:56:58]

Change committed as 162687