Bug 76799

Summary:

display: table-cell does not work properly when applied to img elements

Product:

WebKit

Reporter:

Michael Elsdoerfer <michael>

Component:

CSS

Assignee:

Shezan Baig <shezbaig.wk>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

jchaffraix, michael, robert

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
extended test case	none

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	Details Formatted Diff Diff
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.