RESOLVED INVALID 76799
display: table-cell does not work properly when applied to img elements 
https://bugs.webkit.org/show_bug.cgi?id=76799
Summary display: table-cell does not work properly when applied to img elements
Michael Elsdoerfer
Reported 2012-01-22 10:04:14 PST
img elements are wrapped below below each other, when they should be beside each other: http://jsfiddle.net/elsdoerfer/F6c3N/ Copy of the code here: <!DOCTYPE html> <html> <head> <style> .stripes {display: table; border-collapse: collapse;} .stripes > a {display: table-row; border: 1px solid red} .stripes > a > * { display: table-cell; vertical-align: top; border: 1px solid #000; padding: 10px; } </style> </head> <body> <div class="stripes"> <a href> <img src="http://www.freeimages.co.uk/galleries/nature/weather/thumbs/frost_oak_leaf_winter_218310.jpg" /> </a> <a href> <img src="http://www.freeimages.co.uk/galleries/nature/weather/thumbs/frost_oak_leaf_winter_218310.jpg" /> <img src="http://www.freeimages.co.uk/galleries/nature/weather/thumbs/frost_oak_leaf_winter_218310.jpg" /> <img src="http://www.freeimages.co.uk/galleries/nature/weather/thumbs/frost_oak_leaf_winter_218310.jpg" /> </a> <a href> <img src="http://www.freeimages.co.uk/galleries/nature/weather/thumbs/frost_oak_leaf_winter_218310.jpg" /> </a> </div> </body> </html> I am in fact using Safari 5.1.2 - the latest nightly crashes on Windows, and downloading one takes forever, so I haven't tried any older ones. The same thing happens in Chrome.
Attachments
Patch (6.19 KB, patch)
2012-04-18 14:44 PDT, Shezan Baig 		no flags
DetailsFormatted DiffDiff
extended test case (1.77 KB, text/html)
2012-04-19 07:28 PDT, Shezan Baig 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Shezan Baig
Comment 1 2012-04-18 14:44:52 PDT
Created attachment 137773 [details] Patch initial attempt for review
Shezan Baig
Comment 2 2012-04-18 14:46:54 PDT
CC Julien for review
Shezan Baig
Comment 3 2012-04-18 14:49:17 PDT
For the record, every other browser I've tried does this wrong, and each does it differently from each other (firefox, msie, opera)
Julien Chaffraix
Comment 4 2012-04-18 17:03:13 PDT
Comment on attachment 137773 [details] Patch Is this really limited to <img>? From reading the code, any element with display: table-cell that override createRenderer would display the same issue (like <input>, <fieldset>, ...). Also are we supposed to re-use the same cells for <div> with display: table-cell for example (<div> doesn't override createRenderer)? For the record, IE 9 and FF nightly seem to match each other and put all the images in different cells.
Shezan Baig
Comment 5 2012-04-18 18:26:45 PDT
Yes, this affects any elements that override createRenderer and don't return RenderTableCell when 'display: table-cell' is set. Elements that don't override createRenderer are not affected, because they will never enter this block of code, because the 'child->isTableCell()' check that happens at line 91 (not visible in patch) will return true.
Julien Chaffraix
Comment 6 2012-04-18 21:46:53 PDT
Comment on attachment 137773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137773&action=review (In reply to comment #5) > Yes, this affects any elements that override createRenderer and don't return RenderTableCell when 'display: table-cell' is set. OK, then I would like some more testing to check that it matches the other browsers. You don't need to check all cases (more is better though) but at least 2 or 3. That would give us confidence that this matches other browsers on more grounds. > Elements that don't override createRenderer are not affected, because they will never enter this block of code, because the 'child->isTableCell()' check that happens at line 91 (not visible in patch) will return true. Makes sense. > Source/WebCore/rendering/RenderTableRow.cpp:95 > + if (last && last->isAnonymous() && last->isTableCell() && !last->isBeforeOrAfterContent() && child->style()->display() != TABLE_CELL) { Could you add a comment about that? It took me some time to figure out why this was right. > LayoutTests/fast/table/img-as-table-cell-expected.html:5 > + .filler { background-color: green; } > + .grey { background-color: lightgrey; } I wonder if those are needed for the test to work properly. Shouldn't they be removed as they don't add much or am I missing something? > LayoutTests/fast/table/img-as-table-cell-expected.html:7 > +<div>There should be 3 rows and 3 columns, like this:</div> I like to have the bug information included in any test cases (more points if it's clickable).
Shezan Baig
Comment 7 2012-04-19 07:28:30 PDT
Created attachment 137899 [details] extended test case
Shezan Baig
Comment 8 2012-04-19 07:32:07 PDT
(In reply to comment #4) > For the record, IE 9 and FF nightly seem to match each other and put all the > images in different cells. Can you check the "extended test case"? I seem to get completely different results in IE9, FF nightly and Opera.
Shezan Baig
Comment 9 2012-04-19 10:09:27 PDT
After further testing, it looks like it is not sufficient to just check 'display != TABLE_CELL'. I found the following issues: 1. We actually need to check 'originalDisplay', because the 'display' for form elements gets changed to INLINE_BLOCK in RenderTheme::adjustStyle 2. The following code further below is problematic: RenderTableCell* cell = RenderTableCell::createAnonymousWithParentRenderer(this); The problem with this line is that it creates a RenderTableCell that has the same style as the RenderTableRow, when it really should be getting the style of the child. This problem becomes manifested when the child's style contains a border, i.e. the border appears wrapped around the child element instead of the anonymous table cell. I'll find a better fix for this and upload another patch with more extended tests.
Shezan Baig
Comment 10 2012-04-19 14:46:11 PDT
This turns out to be much more complicated than initially thought. Apart from the layout being correct, we also need to ensure the style propagation is correct, i.e. the table-cell should "steal" certain styles like border etc from the child node. Right now I have it so that I copy the 'surround' object from child->style() to cell->style(), then reset the 'surround' member in child->style() to its default state. However, this causes other problems because child->style() could potentially be shared among multiple elements (CSSStyleSelector::locateSharedStyle). So it looks like we need to clone the child->style() before modifying it, but there doesn't seem to be any way to know whether the style is actually being shared or not. I'll dig around some more and see what else could be done. It might simply be a matter of adding an 'isShared' flag to RenderStyle, or maybe some sort of ref count. I wonder if there will be any overlap with bug 80634
Shezan Baig
Comment 11 2012-04-26 12:04:30 PDT
Marking invalid per discussion with dhyatt on #webkit, who points out that replaced elements can have block-level display, but they're not supposed to be able to be turned into arbitrary block layout display types like flex box or grid or table cell
Note You need to log in before you can comment on or make changes to this bug.