Bug 74874 – REGRESSION (r102040): Wrong column widths when row has colspan and unwrappable text

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

[Robert Hogan]

This is a reduction of an issue seen at build.webkit.org/console

The columns with 'texttesttest' collapse to a tiny width if the colspan is set to 3, but not when set to 2 or 4.

[Robert Hogan]

Created attachment 119891 [details]
Reduction - colspan 2 looks fine

[Robert Hogan]

Created attachment 119892 [details]
Reduction - colspan 3 fails

[Robert Hogan]

Created attachment 122687 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122687 [details]
Patch

Attachment 122687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11268065

New failing tests:
fast/css/min-width-with-spanned-cell.html

[Robert Hogan]

Created attachment 122989 [details]
Patch

[Julien Chaffraix]

Comment on attachment 122989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122989&action=review

Good catch, the code part looks fine.

> LayoutTests/ChangeLog:17
> +        * platform/chromium-linux/fast/table/027-expected.png:
> +        * platform/chromium-linux/fast/table/027-vertical-expected.png:
> +        * platform/chromium-win/fast/table/027-expected.txt:
> +        * platform/chromium-win/fast/table/027-vertical-expected.txt:

Those 2 tests should be disabled on all other platforms to avoid breaking the tree.

> LayoutTests/fast/css/min-width-with-spanned-cell-fixed.html:15
> +<!--When a cell spans an entire row on a table with fixed layout, allow cells containing text to be
> +squeezed smaller than the width of the text.-->

The comment is wrong, it says the opposite of the other test.

It would be nice to add what you expect to see in the pixel dump too for both tests.

> LayoutTests/platform/chromium-win/fast/table/027-vertical-expected.txt:40
> -          RenderTableRow {TR} at (0,28) size 344x744
> +          RenderTableRow {TR} at (0,28) size 344x756

I am concerned about this change. Why is it changing so much when it was a 1px change in an horizontal-writing mode?

Have you checked if our new size matches more closely FF / IE now?

[Julien Chaffraix]

*** Bug 76807 has been marked as a duplicate of this bug. ***

[Robert Hogan]

Comment on attachment 122989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122989&action=review

>> LayoutTests/fast/css/min-width-with-spanned-cell-fixed.html:15
>> +squeezed smaller than the width of the text.-->
> 
> The comment is wrong, it says the opposite of the other test.
> 
> It would be nice to add what you expect to see in the pixel dump too for both tests.

The text is intentional. This is the way it works for fixed width tables afaict.

>> LayoutTests/platform/chromium-win/fast/table/027-vertical-expected.txt:40
>> +          RenderTableRow {TR} at (0,28) size 344x756
> 
> I am concerned about this change. Why is it changing so much when it was a 1px change in an horizontal-writing mode?
> 
> Have you checked if our new size matches more closely FF / IE now?

It's a progression - the image in the last cell no longer extrudes from the table when the height of the view is lower thin the vertical 'width' of the table.

[Robert Hogan]

Created attachment 123590 [details]
Patch

[Julien Chaffraix]

Comment on attachment 123590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123590&action=review

The patch does not apply on ToT because your win expectation file change needs to be rebased.

> LayoutTests/ChangeLog:18
> +        * platform/chromium-linux/fast/table/027-expected.png:
> +          1 px difference - benign
> +        * platform/chromium-linux/fast/table/027-vertical-expected.png:
> +          This is a progression - previously the image in the last cell was
> +          extruding from the table in a 800x600 view.

Nit: Spaces would make those 2 comments stand out more.

> LayoutTests/platform/mac/test_expectations.txt:201
> +BUGWK74874 : fast/table/027.html = TEXT
> +BUGWK74874 : fast/table/027-vertical.html = TEXT

Unless I am missing something, this should also be TEXT+IMAGE as you haven't rebaselined Mac in this patch.

[Robert Hogan]

(In reply to comment #10)
> 
> > LayoutTests/platform/mac/test_expectations.txt:201
> > +BUGWK74874 : fast/table/027.html = TEXT
> > +BUGWK74874 : fast/table/027-vertical.html = TEXT
> 
> Unless I am missing something, this should also be TEXT+IMAGE as you haven't rebaselined Mac in this patch.

The mac bots don't run pixel tests - so will only ever see the TEXT difference.

[Robert Hogan]

Committed r105775: <http://trac.webkit.org/changeset/105775>

[Julien Chaffraix]

(In reply to comment #11)
> (In reply to comment #10)
> > 
> > > LayoutTests/platform/mac/test_expectations.txt:201
> > > +BUGWK74874 : fast/table/027.html = TEXT
> > > +BUGWK74874 : fast/table/027-vertical.html = TEXT
> > 
> > Unless I am missing something, this should also be TEXT+IMAGE as you haven't rebaselined Mac in this patch.
> 
> The mac bots don't run pixel tests - so will only ever see the TEXT difference.

That is not a good reason not to put TEXT+IMAGE. I am sure some developers - apart from myself - are running the tests on Mac with pixel dumps enabled and they will see an unexpected image difference. On top of that, the test_expectations.txt files are annotation on the state of our tests. Here you are doing only half of the annotation for the future maintainer looking at the failure.

[Robert Hogan]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > 
> > > > LayoutTests/platform/mac/test_expectations.txt:201
> > > > +BUGWK74874 : fast/table/027.html = TEXT
> > > > +BUGWK74874 : fast/table/027-vertical.html = TEXT
> > > 
> > > Unless I am missing something, this should also be TEXT+IMAGE as you haven't rebaselined Mac in this patch.
> > 
> > The mac bots don't run pixel tests - so will only ever see the TEXT difference.
> 
> That is not a good reason not to put TEXT+IMAGE. I am sure some developers - apart from myself - are running the tests on Mac with pixel dumps enabled and they will see an unexpected image difference. On top of that, the test_expectations.txt files are annotation on the state of our tests. Here you are doing only half of the annotation for the future maintainer looking at the failure.

Right, but using TEXT+IMAGE means that the bots will report them as unexpected failures in the results.html - even though they're not listed as regressions in the results stdio.

See https://bugs.webkit.org/show_bug.cgi?id=71244#c35 where this came up previously. The lack of a Mac gardener really hurts in this situation.

[Robert Hogan]

Landed results for Qt and Gtk:

http://trac.webkit.org/changeset/105799

[Julien Chaffraix]

> > That is not a good reason not to put TEXT+IMAGE. I am sure some developers - apart from myself - are running the tests on Mac with pixel dumps enabled and they will see an unexpected image difference. On top of that, the test_expectations.txt files are annotation on the state of our tests. Here you are doing only half of the annotation for the future maintainer looking at the failure.
> 
> Right, but using TEXT+IMAGE means that the bots will report them as unexpected failures in the results.html - even though they're not listed as regressions in the results stdio.
> 
> See https://bugs.webkit.org/show_bug.cgi?id=71244#c35 where this came up previously. The lack of a Mac gardener really hurts in this situation.

Ah, that my friend is a good explanation of why we don't want the IMAGE part.

I agree that it is unfortunate that we have to do that just because of snow-leopard.

[Andy Estes]

<rdar://problem/10960651>