Bug 21664 – top and bottom black background line not getting displayed

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Anantha Keesara]

I Steps:
 Go to the attached reduction
 
 II Issue:
 top and bottom black border line not getting displayed
 
 IV Other Browsers:
 IE7: ok
 FF3: ok
 
 V Nightly tested: 37382

[Anantha Keesara]

Created attachment 24415 [details]
reduction.zip

Created attachment 212421 [details]
Patch

[Build Bot]

Comment on attachment 212421 [details]
Patch

Attachment 212421 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1853096

New failing tests:
tables/mozilla_expected_failures/other/empty_cells.html
tables/mozilla_expected_failures/bugs/bug14007-1.html
fast/table/auto-100-percent-width.html
tables/mozilla_expected_failures/bugs/bug72393.html
fast/repaint/table-cell-move.html
tables/mozilla/bugs/bug16012.html
fast/table/empty-cells.html
css2.1/20110323/margin-applies-to-015.htm
ietestcenter/css3/bordersbackgrounds/border-radius-applies-to-007.htm
accessibility/table-detection.html
fast/block/positioning/negative-right-pos.html
fast/table/hittest-tablecell-bottom-edge.html
fast/table/fixed-granular-cols.html
fast/table/hittest-tablecell-right-edge.html
fast/dynamic/insert-before-table-part-in-continuation.html

[Build Bot]

Created attachment 212422 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212421 [details]
Patch

Attachment 212421 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2004478

New failing tests:
tables/mozilla_expected_failures/other/empty_cells.html
tables/mozilla_expected_failures/bugs/bug14007-1.html
fast/table/auto-100-percent-width.html
tables/mozilla_expected_failures/bugs/bug72393.html
fast/repaint/table-cell-move.html
tables/mozilla/bugs/bug16012.html
fast/table/empty-cells.html
css2.1/20110323/margin-applies-to-015.htm
ietestcenter/css3/bordersbackgrounds/border-radius-applies-to-007.htm
accessibility/table-detection.html
fast/block/positioning/negative-right-pos.html
fast/table/hittest-tablecell-bottom-edge.html
fast/table/fixed-granular-cols.html
fast/table/hittest-tablecell-right-edge.html
fast/dynamic/insert-before-table-part-in-continuation.html

[Build Bot]

Created attachment 212423 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 213083 [details]
Patch

[Andrei Bucur]

Comment on attachment 213083 [details]
Patch

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

Hello Gurpreet! Just an informal review.

> Source/WebCore/rendering/AutoTableLayout.cpp:68
> +                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();

Would RenderObject::hasBoxDecorations be more appropriate instead?

(In reply to comment #8)
> (From update of attachment 213083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213083&action=review
> 
> Hello Gurpreet! Just an informal review.
> 
> > Source/WebCore/rendering/AutoTableLayout.cpp:68
> > +                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
> 
> Would RenderObject::hasBoxDecorations be more appropriate instead?

Hi Andrei. Thanks for the review.
hasBoxDecorations will cover border and lot of other properties as well and also will not cover background.

[Build Bot]

Comment on attachment 213083 [details]
Patch

Attachment 213083 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2889095

New failing tests:
fast/table/empty-cells.html
fast/table/auto-100-percent-width.html
tables/mozilla/bugs/bug1818-6.html

[Build Bot]

Created attachment 213085 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213083 [details]
Patch

Attachment 213083 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2894135

New failing tests:
fast/table/empty-cells.html
fast/table/auto-100-percent-width.html
tables/mozilla/bugs/bug1818-6.html

[Build Bot]

Created attachment 213089 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 213101 [details]
Patch

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

Please review.

[Andrei Bucur]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 213083 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213083&action=review
> > 
> > Hello Gurpreet! Just an informal review.
> > 
> > > Source/WebCore/rendering/AutoTableLayout.cpp:68
> > > +                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
> > 
> > Would RenderObject::hasBoxDecorations be more appropriate instead?
> 
> Hi Andrei. Thanks for the review.
> hasBoxDecorations will cover border and lot of other properties as well and also will not cover background.

In RenderBoxModelObject::updateFromStyle

setHasBoxDecorations(hasBackground() || styleToUse->hasBorder() || styleToUse->hasAppearance() || styleToUse->boxShadow());

It includes background. Also, don't you want to do the same if the cell has shadows etc?

(In reply to comment #16)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 213083 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=213083&action=review
> > > 
> > > Hello Gurpreet! Just an informal review.
> > > 
> > > > Source/WebCore/rendering/AutoTableLayout.cpp:68
> > > > +                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
> > > 
> > > Would RenderObject::hasBoxDecorations be more appropriate instead?
> > 
> > Hi Andrei. Thanks for the review.
> > hasBoxDecorations will cover border and lot of other properties as well and also will not cover background.
> 
> In RenderBoxModelObject::updateFromStyle
> 
> setHasBoxDecorations(hasBackground() || styleToUse->hasBorder() || styleToUse->hasAppearance() || styleToUse->boxShadow());
> 
> It includes background. Also, don't you want to do the same if the cell has shadows etc?

Hi Andrei. Ya for shadows etc need to check not sure.

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (From update of attachment 213083 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=213083&action=review
> > > > 
> > > > Hello Gurpreet! Just an informal review.
> > > > 
> > > > > Source/WebCore/rendering/AutoTableLayout.cpp:68
> > > > > +                bool cellHasContent = cell->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding() || cell->style()->hasBackground();
> > > > 
> > > > Would RenderObject::hasBoxDecorations be more appropriate instead?
> > > 
> > > Hi Andrei. Thanks for the review.
> > > hasBoxDecorations will cover border and lot of other properties as well and also will not cover background.
> > 
> > In RenderBoxModelObject::updateFromStyle
> > 
> > setHasBoxDecorations(hasBackground() || styleToUse->hasBorder() || styleToUse->hasAppearance() || styleToUse->boxShadow());
> > 
> > It includes background. Also, don't you want to do the same if the cell has shadows etc?
> 
> Hi Andrei. Ya for shadows etc need to check not sure.

Can someone please review this? Thanks

[Andrei Bucur]

You need to join #webkit on IRC and ask for someone to review. Probably the person to ask is David Hyatt (dhyatt). He is on during the day @ US Pacific Time.

(In reply to comment #19)
> You need to join #webkit on IRC and ask for someone to review. Probably the person to ask is David Hyatt (dhyatt). He is on during the day @ US Pacific Time.

Hi. Can someone please review this. Thanks

(In reply to comment #20)
> (In reply to comment #19)
> > You need to join #webkit on IRC and ask for someone to review. Probably the person to ask is David Hyatt (dhyatt). He is on during the day @ US Pacific Time.
> 
> Hi. Can someone please review this. Thanks

Please review. Thanks

[Simon Fraser (smfr)]

Comment on attachment 213101 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        top and bottom black border line not getting displayed
> +        https://bugs.webkit.org/show_bug.cgi?id=21664
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The table cell's background was not being displayed. Since the table
> +        cell had no child correct offsetWidth was not being set.

The title and explanation of the bug don't quite match, and make this confusing. The title talks about borders, but the fix is related to background color.

(In reply to comment #22)
> (From update of attachment 213101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213101&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        top and bottom black border line not getting displayed
> > +        https://bugs.webkit.org/show_bug.cgi?id=21664
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The table cell's background was not being displayed. Since the table
> > +        cell had no child correct offsetWidth was not being set.
> 
> The title and explanation of the bug don't quite match, and make this confusing. The title talks about borders, but the fix is related to background color.

Thanks Simon for the review. Actually its not border as per the test case , its background. Modified the title as per the bug.

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 213101 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213101&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        top and bottom black border line not getting displayed
> > > +        https://bugs.webkit.org/show_bug.cgi?id=21664
> > > +
> > > +        Reviewed by NOBODY (OOPS!).
> > > +
> > > +        The table cell's background was not being displayed. Since the table
> > > +        cell had no child correct offsetWidth was not being set.
> > 
> > The title and explanation of the bug don't quite match, and make this confusing. The title talks about borders, but the fix is related to background color.
> 
> Thanks Simon for the review. Actually its not border as per the test case , its background. Modified the title as per the bug.

Hi Simon.Can you please review the patch?

[Simon Fraser (smfr)]

Please submit a new patch with the corrected Changelog.

Created attachment 218828 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 218828 [details]
Patch

Clearing flags on attachment: 218828

Committed r160410: <http://trac.webkit.org/changeset/160410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

*** Bug 30949 has been marked as a duplicate of this bug. ***