Bug 15273 – Safari should not render a cell if the <td> is empty

• Component: Tables
• Status: RESOLVED FIXED

[Anantha Keesara]

Safari should not render a column if the <td>, with some width specified, is empty  and if parent table also does not have any width specified.

Attached is the testcase.

[Anantha Keesara]

Created attachment 16373 [details]
Safari should not render a cell if the <td> is empty

WinIE and FIrefox do not render the red border around the third cell.

[Eric Seidel (no email)]

This is probably a dupe of bug 15244.  I might have even seen one other table bug like this today.

[Eric Seidel (no email)]

My guess is this should be a pretty straightforward fix.  We'd just have to check if we have a single empty text child.  If so, don't draw borders.  Possibly not draw backgrounds either, not sure.  Also need to check whitespace handling.  The hard part here is getting a comprehensive test case.

[Arpita Bahuguna]

This issue still seems to be reproducible on WebKit. Based on some initial analysis, it seems that the painting of borders for an empty <td> also seems to be dependant on the doctype of the document:

1. No doctype specified. FF - No broders, Opera - No borders, I.E. - No borders, WebKit - Draws the borders.
2. Invalid doctype (name) e.g. <!doctype> or <!doctype xyz>. FF - No borders, Opera - No borders, I.E. - Draws the borders, WebKit - Draws the broders.
3. Valid doctype (HTML5 or otherwise) - All browsers draw the borders.

Am not sure whether such behavior is part of some specification or not and whether we really need to fix this issue. Would appreciate thoughts on the same.

Created attachment 226303 [details]
Patch

(In reply to comment #6)
> Created an attachment (id=226303) [details]
> Patch

Hi Zalan. Can you please review this? Thanks.

[Darin Adler]

Comment on attachment 226303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226303&action=review

> Source/WebCore/rendering/RenderTableCell.cpp:1329
> +    if (!style().hasBorder() || tableElt->collapseBorders() || (!firstChild() && !tableElt->document().doctype()))

I’m not sure that checking firstChild for null is the right definition of empty. Test coverage is light, which makes it unclear to me.

This should say just document(), not tableElt->document().

Checking doctype is not the correct way to check whether to apply this quirk. The specification may call this "doctype not present or not correct", but it’s actually a compatibility mode. The functions on Document for checking compatibility mode are Document::compatibilityMode, Document::inQuirksMode, Document::inLimitedQuirksMode, and Document::inNoQuirksMode and we should use one of those. Probably inQuirksMode.

> LayoutTests/ChangeLog:13
> +        * fast/table/table-cell-border-doctype-expected.png: Added.
> +        * fast/table/table-cell-border-doctype-expected.txt: Added.
> +        * fast/table/table-cell-border-doctype.html: Added.
> +        * fast/table/table-cell-border-no-doctype-expected.png: Added.
> +        * fast/table/table-cell-border-no-doctype-expected.txt: Added.
> +        * fast/table/table-cell-border-no-doctype.html: Added.

These tests should be done as reference tests, where the expected files are also HTML, not as tests that have pixel results.

The reference file could use a single space or non-breaking space to avoid having the cell be empty. Or it could use explicit properties to turn off the borders.

[Darin Adler]

(In reply to comment #4)
> My guess is this should be a pretty straightforward fix.  We'd just have to check if we have a single empty text child.  If so, don't draw borders.  Possibly not draw backgrounds either, not sure.  Also need to check whitespace handling.  The hard part here is getting a comprehensive test case.

I agree with what Eric said here back in 2008. We might have a text child with no text or some collapsible whitespace that collapsed or maybe that will be omitted from the render tree. We need sufficient test cases to make sure we are handling that case correctly.

(In reply to comment #8)
> (From update of attachment 226303 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226303&action=review
> 
> > Source/WebCore/rendering/RenderTableCell.cpp:1329
> > +    if (!style().hasBorder() || tableElt->collapseBorders() || (!firstChild() && !tableElt->document().doctype()))
> 
> I’m not sure that checking firstChild for null is the right definition of empty. Test coverage is light, which makes it unclear to me.
> 
> This should say just document(), not tableElt->document().
> 
> Checking doctype is not the correct way to check whether to apply this quirk. The specification may call this "doctype not present or not correct", but it’s actually a compatibility mode. The functions on Document for checking compatibility mode are Document::compatibilityMode, Document::inQuirksMode, Document::inLimitedQuirksMode, and Document::inNoQuirksMode and we should use one of those. Probably inQuirksMode.
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/table/table-cell-border-doctype-expected.png: Added.
> > +        * fast/table/table-cell-border-doctype-expected.txt: Added.
> > +        * fast/table/table-cell-border-doctype.html: Added.
> > +        * fast/table/table-cell-border-no-doctype-expected.png: Added.
> > +        * fast/table/table-cell-border-no-doctype-expected.txt: Added.
> > +        * fast/table/table-cell-border-no-doctype.html: Added.
> 
> These tests should be done as reference tests, where the expected files are also HTML, not as tests that have pixel results.
> 
> The reference file could use a single space or non-breaking space to avoid having the cell be empty. Or it could use explicit properties to turn off the borders.

Thanks Darin for the review. Will make the code and test case changes and upload new patch.

(In reply to comment #9)
> (In reply to comment #4)
> > My guess is this should be a pretty straightforward fix.  We'd just have to check if we have a single empty text child.  If so, don't draw borders.  Possibly not draw backgrounds either, not sure.  Also need to check whitespace handling.  The hard part here is getting a comprehensive test case.
> 
> I agree with what Eric said here back in 2008. We might have a text child with no text or some collapsible whitespace that collapsed or maybe that will be omitted from the render tree. We need sufficient test cases to make sure we are handling that case correctly.

Text child with no text for example we create a text node with whitespace (multiple) and then append to table cell?
<script>
function runTest()
{
    var  tableCell = document.getElementById('noChild');
    var text = document.createTextNode("    ");
    tableCell.appendChild(text);
}
</script>
In this case td shows no children so the patch would work. Please correct if I am wrong.

[Darin Adler]

(In reply to comment #11)
> In this case td shows no children so the patch would work.

By “work” you mean that it would omit the borders. Is that what the other browsers do in this case?

(In reply to comment #12)
> (In reply to comment #11)
> > In this case td shows no children so the patch would work.
> 
> By “work” you mean that it would omit the borders. Is that what the other browsers do in this case?

Yes even Firefox omits the border for the above case i.e creating text node with no text.

Created attachment 226658 [details]
Patch

[Darin Adler]

Comment on attachment 226658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226658&action=review

> LayoutTests/ChangeLog:9
> +        * fast/table/table-cell-border-doctype-expected-mismatch.html: Added.
> +        * fast/table/table-cell-border-doctype.html: Added.

Can we do better than expected-mismatch for this test? Or is there no simple markup that will replicate the expected result? Can’t we just do border: 0px or something?

Created attachment 226880 [details]
Patch

(In reply to comment #15)
> (From update of attachment 226658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226658&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * fast/table/table-cell-border-doctype-expected-mismatch.html: Added.
> > +        * fast/table/table-cell-border-doctype.html: Added.
> 
> Can we do better than expected-mismatch for this test? Or is there no simple markup that will replicate the expected result? Can’t we just do border: 0px or something?

Hi Darin. Thanks for the review. Modified the test case.

[Darin Adler]

Comment on attachment 226880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226880&action=review

> LayoutTests/ChangeLog:8
> +        * fast/table/table-cell-border-doctype-expected-mismatch.html: Added.

This is still an expected-mismatch result. I think an expected match would be a much stronger test. Can we make this an expected match test, or is that impractical?

Created attachment 226898 [details]
Patch

(In reply to comment #18)
> (From update of attachment 226880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226880&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * fast/table/table-cell-border-doctype-expected-mismatch.html: Added.
> 
> This is still an expected-mismatch result. I think an expected match would be a much stronger test. Can we make this an expected match test, or is that impractical?

Modified table-cell-border-doctype-expected-mismatch.html to table-cell-border-doctype-expected.html. Tried to generate the same behaviour as table-cell-border-doctype.html. Hope this is fine now :)

[WebKit Commit Bot]

Comment on attachment 226898 [details]
Patch

Clearing flags on attachment: 226898

Committed r165740: <http://trac.webkit.org/changeset/165740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.