[webkit-reviews] review denied: [Bug 15273] Safari should not render a cell if the <td> is empty : [Attachment 226303] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
review:
Bug 15273: Safari should not render a cell if the <td> is empty
https://bugs.webkit.org/show_bug.cgi?id=15273

Attachment 226303: Patch
https://bugs.webkit.org/attachment.cgi?id=226303&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.