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
Created attachment 24415 [details] reduction.zip
Created attachment 212421 [details] Patch
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
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
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
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
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.
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
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
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
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.
(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
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
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?
Please submit a new patch with the corrected Changelog.
Created attachment 218828 [details] Patch
Comment on attachment 218828 [details] Patch Clearing flags on attachment: 218828 Committed r160410: <http://trac.webkit.org/changeset/160410>
All reviewed patches have been landed. Closing bug.
*** Bug 30949 has been marked as a duplicate of this bug. ***