Bug 35632 – htmlediting.cpp : isEmptyTableCell() is incomplete

• Component: HTML Editing
• Status: RESOLVED FIXED

[Roland Steiner]

As currently implemented, isEmptyTableCell() will return true if the passed-in node is table cell, or a BR child of a table cell. It will not test whether there are (other) children present. This causes part of the incorrect behavior described in https://bugs.webkit.org/show_bug.cgi?id=35369.

[Roland Steiner]

Created attachment 49888 [details]
patch - fix isEmptyTableCell

patch fixing the isEmptyTableCell() function

[Darin Adler]

Comment on attachment 49888 [details]
patch - fix isEmptyTableCell

Can't land a change like this without a test case.

[Roland Steiner]

Created attachment 49983 [details]
patch - fix isEmptyTableCell, added layout test

Same patch as before, with added layout test.

[Enrica Casucci]

Comment on attachment 49983 [details]
patch - fix isEmptyTableCell, added layout test

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 55504)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Roland Steiner  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 35632 -  htmlediting.cpp : isEmptyTableCell() is incomplete
> +        https://bugs.webkit.org/show_bug.cgi?id=35632
> +
> +        Correct isEmptyTableCell to check for the presence of other renderer
> +        children.
> +
> +        Test: editing/deleting/delete-br-in-last-table-cell.html
> +
> +        * editing/htmlediting.cpp:
> +        (WebCore::isEmptyTableCell):
> +
>  2010-03-03  Laszlo Gombos  <[e-mail]>
>  
>          Reviewed by Kenneth Rohde Christiansen.
> Index: WebCore/editing/htmlediting.cpp
> ===================================================================
> --- WebCore/editing/htmlediting.cpp	(revision 55504)
> +++ WebCore/editing/htmlediting.cpp	(working copy)
> @@ -863,7 +863,28 @@ bool isTableCell(const Node* node)
>  
>  bool isEmptyTableCell(const Node* node)
>  {
> -    return node && node->renderer() && (node->renderer()->isTableCell() || (node->renderer()->isBR() && node->parentNode()->renderer() && node->parentNode()->renderer()->isTableCell()));     
> +    // Returns true IFF the passed in node is one of:
> +    //   .) a table cell with no children,
> +    //   .) a table cell with a single BR child, and which has no other child renderers, including :before and :after renderers
> +    //   .) the BR child of such a table cell
> +    if (!node)
> +        return false;
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer)
> +        return false;
> +    if (renderer->isBR()) {
> +        renderer = renderer->parent();
> +        if (!renderer)
> +            return false;
> +    }
> +    if (!renderer->isTableCell())
> +        return false;
> +    RenderObject* childRenderer = renderer->firstChild();
> +    if (!childRenderer)
> +        return true;
> +    if (!childRenderer->isBR())
> +        return false;
> +    return !childRenderer->nextSibling();
>  }
>
I don't think this is 100% correct. You should test with markup that contains line breaks.
Those are non rendered text nodes. Make sure your code still returns the expected boolean value if you have something like:

<tr>
<td>
<br>
<span>hello</span>
  
>  PassRefPtr<HTMLElement> createDefaultParagraphElement(Document* document)
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 55504)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> [phone]  Roland Steiner  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 35632 -  htmlediting.cpp : isEmptyTableCell() is incomplete
> +        https://bugs.webkit.org/show_bug.cgi?id=35632
> +
> +        Correct isEmptyTableCell to check for the presence of other renderer
> +        children.
> +        
> +        Layout test checks that deleting a <br> within the last table cell works.
> +
> +        * editing/deleting/delete-br-in-last-table-cell-expected.txt: Added.
> +        * editing/deleting/delete-br-in-last-table-cell.html: Added.
> +        * platform/gtk/Skipped:
> +        * platform/qt/Skipped:
> +
>  2010-03-03  Diego Gonzalez  <[e-mail]>
>  
>          Reviewed by Kenneth Rohde Christiansen.
> Index: LayoutTests/editing/deleting/delete-br-in-last-table-cell-expected.txt
> ===================================================================
> --- LayoutTests/editing/deleting/delete-br-in-last-table-cell-expected.txt	(revision 0)
> +++ LayoutTests/editing/deleting/delete-br-in-last-table-cell-expected.txt	(revision 0)
> @@ -0,0 +1,11 @@
> +Bug 35369 and Bug 35632
> +
> +Executing a delete command when positioned before a BR in a table cell (esp. the last)
> +
> +---
> +1	2
> +3	4
> +---
> +
> +Before: <tbody><tr><td>1</td><td>2</td></tr><tr><td>3</td><td>4<br><br id="lastBR"></td></tr></tbody> 
> +After : <tbody><tr><td>1</td><td>2</td></tr><tr><td>3</td><td>4<br id="lastBR"></td></tr></tbody>
> Index: LayoutTests/editing/deleting/delete-br-in-last-table-cell.html
> ===================================================================
> --- LayoutTests/editing/deleting/delete-br-in-last-table-cell.html	(revision 0)
> +++ LayoutTests/editing/deleting/delete-br-in-last-table-cell.html	(revision 0)
> @@ -0,0 +1,40 @@
> +<html>
> +<head>
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> +<script>
> +function runTest()
> +{
> +    table = document.getElementById("table");
> +    br = document.getElementById("lastBR");
> +    before = document.getElementById("before");
> +    after = document.getElementById("after");
> +    before.innerText = table.innerHTML;
> +    sel = window.getSelection();
> +    sel.setBaseAndExtent(br, 0, br, 0);
> +    deleteCommand();
> +    after.innerText = table.innerHTML.toString();
> +}
> +</script>
> +</head>
> +
> +<body>
> +<p><a href="https://bugs.webkit.org/show_bug.cgi?id=35369">Bug 35369</a> and <a href="https://bugs.webkit.org/show_bug.cgi?id=35632">Bug 35632</a></p>
> +<p>Executing a delete command when positioned before a BR in a table cell (esp. the last)</p>
> +<div contenteditable>
> +---
> +<table id="table"><tr><td>1</td><td>2</td></tr><tr><td>3</td><td>4<br><br id="lastBR"></td></tr></table>
> +---
> +</div>
> +<br>
> +<p>Before: <span id="before">FAILED</span>
> +<br>
> +<p>After : <span id="after">FAILED</span>
> +
> +<script>
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();
> +
> +runTest();
> +</script>
> +</body>
> +</html>
> \ No newline at end of file
> Index: LayoutTests/platform/gtk/Skipped
> ===================================================================
> --- LayoutTests/platform/gtk/Skipped	(revision 55504)
> +++ LayoutTests/platform/gtk/Skipped	(working copy)
> @@ -856,6 +856,7 @@ editing/deleting/delete-br-008.html
>  editing/deleting/delete-br-009.html
>  editing/deleting/delete-br-011.html
>  editing/deleting/delete-br-012.html
> +editing/deleting/delete-br-in-last-table-cell.html
>  editing/deleting/delete-by-word-002.html
>  editing/deleting/delete-character-001.html
>  editing/deleting/delete-first-list-item.html
> Index: LayoutTests/platform/qt/Skipped
> ===================================================================
> --- LayoutTests/platform/qt/Skipped	(revision 55504)
> +++ LayoutTests/platform/qt/Skipped	(working copy)
> @@ -1646,6 +1646,7 @@ editing/deleting/delete-br-010.html
>  editing/deleting/delete-br-011.html
>  editing/deleting/delete-br-012.html
>  editing/deleting/delete-br-013.html
> +editing/deleting/delete-br-in-last-table-cell.html
>  editing/deleting/delete-character-001.html
>  editing/deleting/delete-contiguous-ws-001.html
>  editing/deleting/delete-first-list-item.html

[Roland Steiner]

(In reply to comment #4)
> >  bool isEmptyTableCell(const Node* node)
> >  {
> > -    return node && node->renderer() && (node->renderer()->isTableCell() || (node->renderer()->isBR() && node->parentNode()->renderer() && node->parentNode()->renderer()->isTableCell()));     
> > +    // Returns true IFF the passed in node is one of:
> > +    //   .) a table cell with no children,
> > +    //   .) a table cell with a single BR child, and which has no other child renderers, including :before and :after renderers
> > +    //   .) the BR child of such a table cell
> > +    if (!node)
> > +        return false;
> > +    RenderObject* renderer = node->renderer();
> > +    if (!renderer)
> > +        return false;
> > +    if (renderer->isBR()) {
> > +        renderer = renderer->parent();
> > +        if (!renderer)
> > +            return false;
> > +    }
> > +    if (!renderer->isTableCell())
> > +        return false;
> > +    RenderObject* childRenderer = renderer->firstChild();
> > +    if (!childRenderer)
> > +        return true;
> > +    if (!childRenderer->isBR())
> > +        return false;
> > +    return !childRenderer->nextSibling();
> >  }
> >
> I don't think this is 100% correct. You should test with markup that contains
> line breaks.
> Those are non rendered text nodes. Make sure your code still returns the
> expected boolean value if you have something like:
> 
> <tr>
> <td>
> <br>
> <span>hello</span>

I don't quite follow - in your example, the <br> has a nextSibling, so the function returns false (it's not an empty table cell).

Note that the bug that spawned this implementation (https://bugs.webkit.org/show_bug.cgi?id=35369) also has similar table cell contents: <td>4<br><br></td>. 

Am I misunderstanding your comment, or am I missing the forest for the trees here? ;)

[Enrica Casucci]

(In reply to comment #5)
> (In reply to comment #4)
> > >  bool isEmptyTableCell(const Node* node)
> > >  {
> > > -    return node && node->renderer() && (node->renderer()->isTableCell() || (node->renderer()->isBR() && node->parentNode()->renderer() && node->parentNode()->renderer()->isTableCell()));     
> > > +    // Returns true IFF the passed in node is one of:
> > > +    //   .) a table cell with no children,
> > > +    //   .) a table cell with a single BR child, and which has no other child renderers, including :before and :after renderers
> > > +    //   .) the BR child of such a table cell
> > > +    if (!node)
> > > +        return false;
> > > +    RenderObject* renderer = node->renderer();
> > > +    if (!renderer)
> > > +        return false;
> > > +    if (renderer->isBR()) {
> > > +        renderer = renderer->parent();
> > > +        if (!renderer)
> > > +            return false;
> > > +    }
> > > +    if (!renderer->isTableCell())
> > > +        return false;
> > > +    RenderObject* childRenderer = renderer->firstChild();
> > > +    if (!childRenderer)
> > > +        return true;
> > > +    if (!childRenderer->isBR())
> > > +        return false;
> > > +    return !childRenderer->nextSibling();
> > >  }
> > >
> > I don't think this is 100% correct. You should test with markup that contains
> > line breaks.
> > Those are non rendered text nodes. Make sure your code still returns the
> > expected boolean value if you have something like:
> > 
> > <tr>
> > <td>
> > <br>
> > <span>hello</span>
> 
> I don't quite follow - in your example, the <br> has a nextSibling, so the
> function returns false (it's not an empty table cell).
> 
> Note that the bug that spawned this implementation
> (https://bugs.webkit.org/show_bug.cgi?id=35369) also has similar table cell
> contents: <td>4<br><br></td>. 
> 
> Am I misunderstanding your comment, or am I missing the forest for the trees
> here? ;)

When you have new lines in the markup, the DOM tree contains text nodes that are not rendered.
The DOM tree for 
<br><span>hello</span>
is different from the DOM tree for
<br>
<span>hello</span>.
In this case between <br> and <span> there is a non rendered text node.
I hope this helps clarify my comment.

[Roland Steiner]

Created attachment 50872 [details]
patch - change isEmptyTableCell() and layout tests

Thanks for the clarification, now I understood what you meant. Uploaded a new patch version that skips unrendered nodes.

[Kent Tamura]

Comment on attachment 50872 [details]
patch - change isEmptyTableCell() and layout tests

LayoutTests/ChangeLog:16
 +          * platform/qt/Skipped:
Why is the test skipped in GTK and Qt?

[Ojan Vafai]

Comment on attachment 50872 [details]
patch - change isEmptyTableCell() and layout tests

This test shouldn't be platform specific. r=me if you remove adding the test to the skipped lists. Please consider doing the other comments below as well.

WebCore/editing/htmlediting.cpp:870
 +      //   .) the BR child of such a table cell
Side note: This seems wrong to me. Seems like it will lead to confusing calling code. In either case, your patch isn't adding this. Might deserve a FIXME though if you agree that this is string. I'd be happier if this were called isEmptyTableCellOrInsideOf...or something like that. Not asking you to change this for this patch though. Just a cleanup to consider for a followup patch.

LayoutTests/editing/deleting/delete-br-in-last-table-cell.html:41
 +  \ No newline at end of file
please add a newline.

LayoutTests/editing/deleting/delete-br-in-last-table-cell.html:35
 +      layoutTestController.dumpAsText();
dump_as_markup didn't exist when you wrote this patch, but it would be appropriate for this test. tony can show you how to write one if you have trouble (you just include the dump_as_markup.js file as a script element).

[Roland Steiner]

(In reply to comment #9)
> (From update of attachment 50872 [details])
> This test shouldn't be platform specific. r=me if you remove adding the test to the skipped lists. Please consider doing the other comments below as well.

I of course agree that the code changed here isn't be platform-specific. The reason I added the test to the Skipped lists is that all similar tests that use the same functionality used in the test to arrive at isEmptyTableCell() - i.e., execCommand("delete") - are skipped as well.

So in order to not include the test in the Skipped list of those platforms, AFAICT the test would need to be rewritten completely (?)

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 50872 [details] [details])
> > This test shouldn't be platform specific. r=me if you remove adding the test to the skipped lists. Please consider doing the other comments below as well.
> 
> I of course agree that the code changed here isn't be platform-specific. The reason I added the test to the Skipped lists is that all similar tests that use the same functionality used in the test to arrive at isEmptyTableCell() - i.e., execCommand("delete") - are skipped as well.
> 
> So in order to not include the test in the Skipped list of those platforms, AFAICT the test would need to be rewritten completely (?)

I see. That makes sense. I took another look at the patch. One nit: It would be nice to add a few line breaks logically grouping the code in isEmptyTableCell. It's a bit hard to read as one big chunk like that.

r=me either way

[Roland Steiner]

Broke up the function a bit and added further comments for clarification.

Changed the test to use dump_as_markup.

Landed as r60936.