Summary: | REGRESSION(r140907): Incorrect baseline for cells with media content during load | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Scherkus <scherkus> | ||||||||
Component: | Tables | Assignee: | Robert Hogan <robert> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, esprehn+autocc, jchaffraix, nbarth, ojan.autocc, robert, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andrew Scherkus
2013-01-30 10:44:01 PST
While the media is being loaded the cell can come up with a very high firstLineBoxBaseline() value. robert: I'm not very familiar with layout so bear with me :) is having a very high firstLineBoxBaseline() value a bad thing? i.e., is this a bug that we needs fixing? (In reply to comment #2) > robert: I'm not very familiar with layout so bear with me :) is having a very high firstLineBoxBaseline() value a bad thing? i.e., is this a bug that we needs fixing? It is - and I've assigned it to myself! Hopefully a fix will be along soon.. Created attachment 186903 [details]
Patch
Created attachment 187423 [details]
Patch
Comment on attachment 187423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187423&action=review > Source/WebCore/rendering/RenderTableCell.cpp:258 > + layoutBlock(cellWidthChanged()); I don't think this is the right way. First if you don't call setNeedsLayout, this will ASSERT in debug. Second, baseline computation are done by RenderTableSection (layoutRows) so this should be handled there. By lastly, I don't think I understand the issue. The image loading should trigger a relayout (including the row / section so that we recompute the baseline appropriately) and if that's not the case, then we should fix that, not force 2 layout phases every time this function is called. > LayoutTests/http/tests/css/vertical-align-baseline-after-image-load-expected.html:12 > + return "http://127.0.0.1:8000/resources/load-and-stall.php"; Tests using the http server should be inside the http/tests/ directory or else the server may not be started. Comment on attachment 187423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187423&action=review > Source/WebCore/rendering/RenderTableCell.cpp:250 > + // If we have replaced content, the intrinsic height of our content may have changed since the last time we laid out. If that's the case the intrinsic padding we used I think this first sentence should give you the direction to drill. Why wasn't the intrinsic height recomputed? Is there a way for us to properly relayout *before* we do the baseline computation so that we get the right intrinsic padding with one layout? (In reply to comment #6) > By lastly, I don't think I understand the issue. The image loading should trigger a relayout (including the row / section .. so that we recompute the baseline appropriately) This is my row before the image in the second cell (B) has loaded: -------------------- | --- | | | | | | | | | | | | | | | | __ | | | A | | | B| | | |___| | |__| | -------------------- The height of A has set the baseline on the row so B sits on the baseline. That space above B is 'intrinsic padding' used to push B down to the same baseline as A and is treated the same as CSS padding during layout. When the image B loads we layout again, and in the broken case get this: -------------------- | | | | | | | | | | ___ | __ | | | A | | | B| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | |___| | |__| | -------------------- The padding in the second cell was used during layout to calculate the logical top of B, but it is wrong to do that because that padding was used to force the old B down to the same baseline as A. Since we didn't know before calling layoutBlock() that B would increase height this way and since the padding has now fed into the logical top of every linebox in the cell we have to correct the offsets used in the cell's children with another layout using the correct padding. I think we can account for this in layoutRows() as you suggest - but it didn't feel right to me to carry around the wrong linebox positions and cellBaselinePosition() for the second cell until the next arbitrary layout fixes them for us. (In reply to comment #7) > (From update of attachment 187423 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187423&action=review > > > Source/WebCore/rendering/RenderTableCell.cpp:250 > > + // If we have replaced content, the intrinsic height of our content may have changed since the last time we laid out. If that's the case the intrinsic padding we used > > I think this first sentence should give you the direction to drill. Why wasn't the intrinsic height recomputed? Because this is the first time the image has gotten layout since it finished loading. Once we've laid out the cell we'll know the height of the loaded image for the first time. >Is there a way for us to properly relayout *before* we do the baseline computation so that we get the right intrinsic padding with one layout? No, I believe not. See my previous comment for the way intrinsic padding is baked into the baseline and indeed the block position of every descendant of the cell. Friendly ping! Chrome is making a release soon and I'd hate for the regression to make it out to the stable branch. Anything I should be wary of if I revert 140907 in our release branch while the proper fix is sorted out on trunk? It looks like it was fixing a different regression from ~8 months ago: http://trac.webkit.org/changeset/120616 @scherkus: a WebKit bug is not the place to discuss / complain about Chromium releases. File a Chromium bug and CC me on it. (and yes, there is a string of changes to revert if you don't want to introduce more regressions) I wanted to understand the issue in depth by debugging fast/css/vertical-align-baseline-rowspan-012.html. However the test passes from me under ToT Chromium DRT but not under Chrome Canary. No clue really what could cause this difference in behavior though. jchaffraix: not discussing or complaining, just looking for guidance w.r.t. how painful it'll be to revert vs. the proper fix I've cc'd you on the relevant bug Created attachment 190794 [details]
Patch
Comment on attachment 190794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190794&action=review Sigh. > Source/WebCore/rendering/RenderTableCell.cpp:255 > + if ((va == BASELINE || va == TEXT_BOTTOM || va == TEXT_TOP || va == SUPER || va == SUB || va == LENGTH) && cellBaselinePosition() > section()->rowBaseline(rowIndex())) { This should need some helper function as it's now repeated 3 times: va == BASELINE || va == TEXT_BOTTOM || va == TEXT_TOP || va == SUPER || va == SUB || va == LENGTH Also all these case should be tested below (only BASELINE is AFAICT) or else nothing prevents us from breaking them later on. Committed r145305: <http://trac.webkit.org/changeset/145305> This may have caused a performance regression; please see: Bug 112125 - 1.3% mac-release-10.6-webkit-latest/intl2/times/t change after rev 145300 |