Bug 108357 – REGRESSION(r140907): Incorrect baseline for cells with media content during load

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Andrew Scherkus]

Bisected builds to the following Chromium and WebKit ranges:
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=179129%3A179136
http://trac.webkit.org/log/trunk/?rev=140956&stop_rev=140854&verbose=on&limit=10000

jchaffraix/robert: so far I think http://trac.webkit.org/changeset/140907 might be the suspect, but I haven't attempted reverting that change and testing locally yet

Website that reproduces the issue:
http://src.chromium.org/viewvc/chrome/trunk/src/content/test/data/media/blackwhite.html

The vertical height of the cells keep expanding as I change the src attribute of the <video> (click on the various buttons).

Also reproducible with Safari + WebKit nightly r141277.

[Robert Hogan]

While the media is being loaded the cell can come up with a very high firstLineBoxBaseline() value.

[Andrew Scherkus]

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?

[Robert Hogan]

(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..

[Robert Hogan]

Created attachment 186903 [details]
Patch

[Robert Hogan]

Created attachment 187423 [details]
Patch

[Julien Chaffraix]

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.

[Julien Chaffraix]

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?

[Robert Hogan]

(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.

[Robert Hogan]

(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.

[Andrew Scherkus]

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

[Julien Chaffraix]

@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)

[Julien Chaffraix]

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.

[Andrew Scherkus]

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

[Robert Hogan]

Created attachment 190794 [details]
Patch

[Julien Chaffraix]

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.

[Robert Hogan]

Committed r145305: <http://trac.webkit.org/changeset/145305>

[Nils Barth]

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