[Webkit-unassigned] [Bug 81084] Rendering replaced elements with percentage height within a table cell (with auto height)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81084


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #132759|review?                     |
               Flag|                            |




--- Comment #7 from Julien Chaffraix <jchaffraix at webkit.org>  2012-03-20 09:54:26 PST ---
(From update of attachment 132759)
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> </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> </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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.