Bug 81084

Created attachment 131797 [details] Screenshot for failure case taken on Safari

Created attachment 131816 [details] Local test case for reproducing the issue Please fetch passportTest.html. This will internally fetch iframe.html (in the same path).

Created attachment 131862 [details] Another reduction using an image instead of an iframe It seems like the replaced element is overflowing the cell in both cases. Likely related to how we layout our table.

If the containing block (i.e. the table cell) does not have a fixed width, should the replaced element (with percent height/width) then have to constrict itself?

Created attachment 132759 [details] Patch

Proposed patch has been uploaded. Proper minPreferredLogicalWidth should be set for replaced elements when placed under table cells with auto height. A new layout test and test result have been added for Windows port.

Comment on attachment 132759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132759&action=review The patch cannot be applied by our tools due to the SVN property (which doesn't look right to me anyway). > Source/WebCore/rendering/RenderReplaced.cpp:586 > + while (cb && !cb->isRenderView() && (cb->style()->height().isAuto() || cb->style()->height().isPercent())) { > + if (cb->isTableCell()) > + isTableCellHeightAuto = true; > + cb = cb->containingBlock(); > + } You are making hasRelativeDimensions O(n) instead of O(1) as it was before. I think that's fine as it's rarely used. Please add a break statement if you find a table cell as it's useless to continue walking your ancestor's chain. > Source/WebCore/rendering/RenderReplaced.cpp:590 > + return (style()->width().isPercent() || (style()->height().isPercent() && !isTableCellHeightAuto) > + || style()->maxWidth().isPercent() || (style()->maxHeight().isPercent() && !isTableCellHeightAuto) > + || style()->minWidth().isPercent() || (style()->minHeight().isPercent() && !isTableCellHeightAuto)); I don't understand why you single out height here. If you encountered a cell's containing block with auto or percent width, wouldn't you need the same exception? Your testing would need to cover that too. > Source/WebCore/rendering/RenderReplaced.h:41 > + virtual bool hasRelativeDimensions() const; Add the 'OVERRIDE' keyword at the end. > LayoutTests/ChangeLog:9 > + * platform/win/fast/replaced/table-cell-width-expected.txt: Added. I don't think the output is platform-specific as you have a done a good job of making it dumpAsText. > LayoutTests/fast/replaced/table-cell-width.html:35 > +function parsePixelValue(str) Used only by is75PercentOf so unused. > LayoutTests/fast/replaced/table-cell-width.html:44 > +function is75PercentOf(expression75, expression100) Unused function AFAICT. > LayoutTests/fast/replaced/table-cell-width.html:126 > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } No need for this. DRT dumps after the load event listener was run by default. > LayoutTests/fast/replaced/table-cell-width.html:133 > +<table><tr><td id="canvas-75"><canvas style="background-color: #00ff00; height: 75%;"></canvas></td><td>This is the second table cell</td><td style="background-color: grey;" width="100%"><span>&nbsp;</span></td></tr></table> > +<table><tr><td id="canvas-100"><canvas style="background-color: #00ff00; height: 100%;"></canvas></td><td>This is the second table cell</td><td style="background-color: grey;" width="100%"><span>&nbsp;</span></td></tr></table> Please use some CSS class selector on those. That would make it way easier to read the output. For example: .75percentHeight { height: 75% } canvas { background-color: #0F0; } ... > LayoutTests/platform/win/fast/replaced/table-cell-width-expected.txt:30 > +This is the second table cell   > +This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + > +This is the second table cell   > + > +This is the second table cell   > + This is the second table cell   > + This is the second table cell   > +Button This is the second table cell   > +Button This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   > + This is the second table cell   Not a huge fan of those. You could wrap all of them in an element and remove it before at the end of your 'load' event listener. > LayoutTests/platform/win/fast/replaced/table-cell-width-expected.txt:36 > +PASS getWidth('canvas-75') is '224px' This test (among others) is failing in FF. I haven't thought much about it but setting the size on your tables (or wrapping them in a fixed sized container) may help here.

(In reply to comment #7) Thanks for reviewing the patch Julien. I shall incorporate the changes as suggested by you and upload another patch shortly. > The patch cannot be applied by our tools due to the SVN property (which doesn't look right to me anyway). Could you please let me know as to why the patch could not be applied and how to rectify the same. > > > Source/WebCore/rendering/RenderReplaced.cpp:590 > > + return (style()->width().isPercent() || (style()->height().isPercent() && !isTableCellHeightAuto) > > + || style()->maxWidth().isPercent() || (style()->maxHeight().isPercent() && !isTableCellHeightAuto) > > + || style()->minWidth().isPercent() || (style()->minHeight().isPercent() && !isTableCellHeightAuto)); > > I don't understand why you single out height here. If you encountered a cell's containing block with auto or percent width, wouldn't you need the same exception? > > Your testing would need to cover that too. > Agree on the above and after further investigation would like to generalize the bug as "Rendering replaced elements with relative dimensions within a table cell (with auto\percent dimensions)".

Created attachment 134576 [details] Patch

Proposed patch has been uploaded incorporating previous review comments. Three new layout test cases (platform specific) have been added in LayoutTests/fast/replaced path. Expected files for the same have currently been updated for the Qt port alone (under LayoutTests/platform/qt/fast/replaced). Need to update for the remaining ports. Image height returns different values on different ports and hence the expected result needs to be generated for each. Expected files (image and text) for the existing layout test case: width100percent-image.html (under LayoutTests/fast/replaced) need to be modified. Have added the new expected files for the Qt port (under LayoutTests/platform/qt/fast/replaced). Need to update for the remaining ports. The fix has now been modified to take both the relative width and height into consideration.

Comment on attachment 134576 [details] Patch Attachment 134576 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12173403 New failing tests: fast/replaced/width100percent-image.html

Created attachment 134590 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 134576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134576&action=review > Source/WebCore/ChangeLog:7 > + ChangeLogs SHOULD be filled. Look at other ChangeLogs on how to do that. > Source/WebCore/rendering/RenderReplaced.cpp:430 > + // In case containing block is a table with auto/percent dimensions, the max preferred logical width should be used This definitely needs a 'why' explanation. Normally table cells shrink to fit their internal content. Here you are forcing them to use our preferred logical width. > Source/WebCore/rendering/RenderReplaced.cpp:432 > + RenderBlock* cb = this->containingBlock(); Please don't abbreviate. > Source/WebCore/rendering/RenderReplaced.cpp:433 > + bool isTableAuto = false; This name is bad: auto in table has different meaning than for any other element (auto table layout). > Source/WebCore/rendering/RenderReplaced.cpp:436 > + if (cb->isTable()) { Shouldn't we be looking for a table cell not a table here? > Source/WebCore/rendering/RenderReplaced.cpp:443 > + if (hasRelativeDimensions() && !isTableAuto) We really would like our containing block's chain crawling to happen only if we *do* have relative dimension. This means that the code above should be moved in an helper function: hasTableWithRelativeDimensionsInContainingBlockChain or something. > LayoutTests/ChangeLog:3 > + Rendering replaced elements with relative dimensions within a table cell (with auto\percent dimensions) The bug name could made more explicit here: Fix rendering of replaced elements with relative dimensions within a table cell. > LayoutTests/ChangeLog:15 > + * platform/qt/fast/replaced/width100percent-image-expected.png: > + * platform/qt/fast/replaced/width100percent-image-expected.txt: You should explain if / why this is a progression. This test needs to be skipped on all other platforms. This is what is making the cr-linux EWS unhappy. Currently, you should test_expectations.txt for Chromium and Skipped for all other platforms (mac, mac-wk2, qt, gtk, efl, win-cairo) > LayoutTests/fast/replaced/table-cell-width-percent.html:102 > + var elements = document.getElementsByClassName('remove'); > + for (i = 0; i < elements.length; i++) { > + if (elements[i]) > + elements[i].innerText = ""; > + } Very neat trick. I think you could remove the leading empty lines by just doing: var elements = document.getElementByTagName('table'); for (var i = 0; i < elements.length; ++i) elements[i].parentNode.removeChild(elements[i]); > LayoutTests/platform/qt/fast/replaced/table-cell-width-percent-expected.txt:30 > +FAIL getWidth('canvas-td-width-percent') should be 300px. Was 530px. Why is it failing? What are we testing here that we do not match? Is this expected?

(In reply to comment #13) Thanks for once again reviewing the patch Julien. Would appreciate your opinion on the following: > > > Source/WebCore/rendering/RenderReplaced.cpp:436 > > + if (cb->isTable()) { > > Shouldn't we be looking for a table cell not a table here? > The logic here is that if we get any containing block (up to table i.e. either table cell or table) with fixed dimensions then our replaced inner element (with relative dimensions) should get rendered with respect to those fixed dimensions. Otherwise, we should go ahead with the (intrinsic) size of the replaced element and expand our (relative) table cell dimensions accordingly. For scenarios which have a fixed (dimension) table but a relative table cell containing relative replaced element within, the earlier patch would have rendered the replaced element with its full size and would have also expanded the table cell accordingly. But since the table has fixed dimensions this would have resulted in an incorrect layout. Thus, we should check for any fixed containing block up to table. Also, the previous patch with the tableCell() check was failing an existing layout test case table-percent-width.html (under fast/replaced) for the cases which have a fixed table width. table-percent-width-actual.txt with tableCell() check instead of table() FAIL getWidth('img-3') should be 40px. Was 105px. FAIL getHeight('img-3') should be 40px. Was 105px. FAIL getWidth('img-5') should be 40px. Was 105px. > > LayoutTests/platform/qt/fast/replaced/table-cell-width-percent-expected.txt:30 > > +FAIL getWidth('canvas-td-width-percent') should be 300px. Was 530px. > > Why is it failing? What are we testing here that we do not match? Is this expected? This is the case in which the replaced element has height 100% and the containing table cell has width 100%. The only restriction is on the width of the table which is 600px. Here too we would be expecting a width of 300px (the intrinsic size width) but instead get 530px which is due to the table cell's 100% width being considered (wrt table width 600px). This behavior is the same with or without my fix. Also, we get similar values on Firefox as well. I shall incorporate the remaining review comments into the next patch.

Created attachment 135815 [details] Patch

Proposed patch uploaded. New helper function has been added for checking whether any containing block is a table with relative dimensions. ChangeLog has been modified with detailed description.

Comment on attachment 135815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135815&action=review Sorry for the delay. It looks better, more comments but I like the new change. > Source/WebCore/rendering/RenderReplaced.cpp:561 > + bool isTableRelative = false; I still don't like this variable name. Whenever you have a variable, think of how you would explain its role in English. Reading this one makes me think that |this| is a relative (sized? positioned?) table which it is not! Better namings: hasAutoOrPercentContainingTable, hasContainingTableWithRelativeDimensions > Source/WebCore/rendering/RenderReplaced.cpp:564 > + while (containingBlock && !containingBlock->isRenderView() && (containingBlock->style()->height().isAuto() || containingBlock->style()->height().isPercent() > + || containingBlock->style()->width().isAuto() || containingBlock->style()->width().isPercent())) { It's difficult to read because of all the conditions. You could split the hasRelativeWidthOrHeight logic into its own static function (taking the style) and maybe even switch to a |for| loop here: for (const RenderBlock* containingBlock = this->containingBlock; containingBlock && !containingBlock->isRenderView() && hasRelativeWidthOrHeight(containingBlock->style()); containingBlock = containingBlock->containingBlock()) (it should be |const| as you don't expect it to be modified) > LayoutTests/ChangeLog:8 > + * fast/replaced/table-cell-width-auto.html: Added. You are missing the expected results for your 3 tests.

Created attachment 137514 [details] Proposed patch Julien, have made the changes as suggested by you. Thanks for the review. Have added the expected files for the newly added test cases for the qt port. The image height gives different values on qt as compared against windows (a difference of 5px); hence have made the expected files port specific. Also, have added the helper function hasRelativeWidthOrHeight() which checks whether the style contains auto/percent dimensions. As this function is not specific to RenderReplaced; have moved it outside the class.

Created attachment 137524 [details] Proposed patch Uploaded another patch making the helper function hasRelativeWidthOrHeight() static.

Comment on attachment 137524 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=137524&action=review > Source/WebCore/rendering/RenderReplaced.cpp:562 > + bool hasRelativeDimensions = false; > + > + if (style->height().isAuto() || style->height().isPercent() || style->width().isAuto() || style->width().isPercent()) > + hasRelativeDimensions = true; > + > + return hasRelativeDimensions; This could be simplified a lot: return style->height().isAuto() || style->height().isPercent() || style->width().isAuto() || style->width().isPercent(); > LayoutTests/ChangeLog:34 > + * platform/wincairo/Skipped: You can always re-organize the entries to avoid duplicated text. I find that it has the benefit of making the ChangeLog more cleaner and more readable (smaller). > LayoutTests/platform/qt/fast/replaced/table-cell-width-percent-expected.txt:44 > +FAIL getWidth('input-image-td-width-percent') should be 100px. Was 530px. Please consider filing a follow-up bug on having a test with some failing results.

Created attachment 137859 [details] Proposed patch Have made changes according to the latest review comments.

Attachment 137859 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:368: Duplicate or ambiguous expectation. editing/undo/undo-smart-delete-reversed-selection.html [test/expectations] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 137859 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=137859&action=review The style complains is just an existing issue on the platform. I overlooked the tests in my previous review (my apologies). Corrected now... > Source/WebCore/rendering/RenderReplaced.cpp:556 > + ASSERT(style); You don't need this ASSERT, RenderObject::style() cannot be NULL AFAICT (sorry I forgot to mention it last time). > Source/WebCore/rendering/RenderReplaced.cpp:558 > + return (style->height().isAuto() || style->height().isPercent() || style->width().isAuto() || style->width().isPercent()); Unneeded parenthesis. > LayoutTests/ChangeLog:33 > + * platform/qt/fast/replaced/table-cell-width-auto-expected.txt: Added. > + * platform/qt/fast/replaced/table-cell-width-fixed-expected.txt: Added. > + * platform/qt/fast/replaced/table-cell-width-percent-expected.txt: Added. > + Expected results for the newly added test cases for qt port. Those results are not port specific. They should be moved new to the test (again I missed that in the previous review). > LayoutTests/fast/replaced/table-cell-width-auto.html:120 > +.greenBgColor { > + background-color: green; > +} > +.greyBgColor { > + background-color: grey; > +} Do we *really* need those? > LayoutTests/fast/replaced/table-cell-width-auto.html:127 > +<table width="600px"><tr><td id="canvas-height-percent"><canvas class="height100percent greenBgColor"></canvas></td><td><span class="remove">This is the second table cell</span></td><td width="100%" greyBgColor"><span class="remove">&nbsp;</span></td></tr></table> All your table are 600px width, you should replace that with a tag selector: table { width: 600px; } > LayoutTests/fast/replaced/table-cell-width-auto.html:128 > +<table width="600px"><tr><td id="canvas-width-percent"><canvas class="width100percent greenBgColor"></canvas></td><td><span class="remove">This is the second table cell</span></td><td width="100%" greyBgColor"><span class="remove">&nbsp;</span></td></tr></table> <td width="100%" greyBgColor"> <- there are some trailing unrelated stuffs (not repeated). The non-breaking space is unneeded AFAICT. As is the remove class. > LayoutTests/fast/replaced/table-cell-width-fixed.html:3 > +<title>Test for Buzilla Bug 81084: Fix rendering of replaced elements with relative dimensions within a table cell.</title> My preference is for the bug id to be dumped in the output as it makes it easier for port maintainer to find errors. Bonus point if it includes the URL in a clickable link. > LayoutTests/fast/replaced/table-cell-width-fixed.html:8 > +if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > +} AFAICT you don't need that as js-test-pre.js already does it for you. > LayoutTests/fast/replaced/table-cell-width-fixed.html:14 > + if (!element) { > + return null; > + } Style violation. Do you really want this to fail silently if you have no elements? > LayoutTests/fast/replaced/table-cell-width-fixed.html:101 > + elements[0].parentNode.removeChild(elements[0]); > + elements = document.getElementsByTagName('table'); We prefer tests to follow WebKit coding style (4 spaces tab).

Comment on attachment 137524 [details] Proposed patch Cleared Julien Chaffraix's review+ from obsolete attachment 137524 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 138082 [details] Patch

Attachment 138082 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:359: Duplicate or ambiguous expectation. editing/undo/undo-smart-delete-reversed-selection.html [test/expectations] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Uploaded another patch with suggested changes.

Comment on attachment 138082 [details] Patch Attachment 138082 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12470228

Created attachment 138314 [details] Patch

Attachment 138314 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:359: Duplicate or ambiguous expectation. editing/undo/undo-smart-delete-reversed-selection.html [test/expectations] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 138314 [details] Patch I am having second thoughts on the approach as it forces us to walk our ancestor's chain every time you have to recompute your preferred widths which may end up being expensive. Also you are forcing the replaced element to be it's (maximum) preferred size. IIRC images can be downscaled as long as the intrinsic ratio is kept. I wonder if this couldn't be handled in the table layout code without intruding in the common code.

(In reply to comment #31) > (From update of attachment 138314 [details]) > I am having second thoughts on the approach as it forces us to walk our ancestor's chain every time you have to recompute your preferred widths which may end up being expensive. Agree, especially if you don't have a table as parent this would be quite an unnecessary walk. But we require to walk the tree either from RenderReplaced (for containing block) or from Table (for its children). One possibility could be that we maintain some flag in RenderReplaced/Table but need to analyze how scenarios in which the style for the concerned elements changes dynamically, is going to affect our layout. > > Also you are forcing the replaced element to be it's (maximum) preferred size. IIRC images can be downscaled as long as the intrinsic ratio is kept. I am sorry but could you please elaborate more on "images can be downscaled as long as the intrinsic ratio is kept". > > I wonder if this couldn't be handled in the table layout code without intruding in the common code. Have tried to figure out ways of handling the width manipulation in RenderTable/TableCell itself but even there we would have to walk the tree to find a replaced child.

Comment on attachment 138314 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

Created attachment 463606 [details] Safari 16.1 matches other browsers As can be seen from attached picture, I am unable to reproduce this bug and Safari matches will all browsers. Thanks!