Bug 15273

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.

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

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.

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.

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.

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

(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

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.

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 :)

Comment on attachment 226898 [details] Patch Clearing flags on attachment: 226898 Committed r165740: <http://trac.webkit.org/changeset/165740>

All reviewed patches have been landed. Closing bug.