Bug 110545 – Increase the max preferred width of tables to 1000000

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Increase the max preferred width of tables to 1000000

[Ojan Vafai]

Created attachment 189661 [details]
Patch

[Emil A Eklund]

Comment on attachment 189661 [details]
Patch

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

> Source/WebCore/rendering/TableLayout.h:47
> +    const static int tableMaxWidth = 1000000;

Why did you pick 1,000,000? Still seems really small.

> LayoutTests/fast/table/large-shrink-wrapped-width.html:24
> +<!-- The inner div should fill the container. It doesn't right now because we artificially limit the tables maxPreferredLogicalWidth to 1000000.

I really don't think this test benefits from being done as a check-layout test.

shouldBe("tableElement.offsetWidth", container.scrollWidth); (or clientWidth depending on the test)

would be less code, a lot simpler and avoids the need for hard coded expectations.

[Benjamin Poulain]

Comment on attachment 189661 [details]
Patch

> > LayoutTests/fast/table/large-shrink-wrapped-width.html:24
> > +<!-- The inner div should fill the container. It doesn't right now because we artificially limit the tables maxPreferredLogicalWidth to 1000000.
> 
> I really don't think this test benefits from being done as a check-layout test.
> 
> shouldBe("tableElement.offsetWidth", container.scrollWidth); (or clientWidth depending on the test)
> 
> would be less code, a lot simpler and avoids the need for hard coded expectations.

I completely agree, the test could be better IMHO. It is extremely opaque.

[Emil A Eklund]

(In reply to comment #2)
Thinking of this some more I realize why you did it this way, hard-coded values might be unavoidable in this case. I think moving all of the width-styles to be inline would go a long way towards making the test easier to understand though.

Another option might be to set the width of the table or a cell and then checking the offsetWidth of the table to see if it matches the set value. Still has to deal with border/padding/margins though...

[Ojan Vafai]

Comment on attachment 189661 [details]
Patch

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

>> Source/WebCore/rendering/TableLayout.h:47
>> +    const static int tableMaxWidth = 1000000;
> 
> Why did you pick 1,000,000? Still seems really small.

I figure it's still a lot larger than 15000 and really we'll just get rid of this once we enable SATURATED_LAYOUT_ARITHMETHIC everywhere, so I didn't think too much about it, especially since there are tests that actually do overflow if you pick a large enough value.

[Ojan Vafai]

(In reply to comment #4)
> (In reply to comment #2)
> Thinking of this some more I realize why you did it this way, hard-coded values might be unavoidable in this case. 

Right. The parent's scrollWidth and the table's offsetWidth are both wrong (and match each other) at tip of tree.

> I think moving all of the width-styles to be inline would go a long way towards making the test easier to understand though.

I can do that if you really want, but I think it makes the test hard to make sense of because you can't easily tell what's different between each test case because there is now all the copy-pasted widths everywhere.

> Another option might be to set the width of the table or a cell and then checking the offsetWidth of the table to see if it matches the set value. Still has to deal with border/padding/margins though...

The tricky thing here is that the width of the table needs to be 100% to hit the bad codepath. This tableMaxWidth thing is a bit of a lie really. It only applies in really obscure edge cases it turns out. Most of the time there's no limit on how big your table can be. It was actually difficult to write a test case where this is exposed to web content.

[Ojan Vafai]

Created attachment 189806 [details]
inline some widths

[Ojan Vafai]

I inlined all the widths except for the container width. Does that work?

Also, I'm fine picking a number larger than 1000000, but I'd rather we just wait until we enable SATURATED_LAYOUT_ARITHMETHIC, so we can do the proper fix and get rid of this variable entirely.

[Emil A Eklund]

Comment on attachment 189806 [details]
inline some widths

Makes sense. Thanks for updating the test.

[Ojan Vafai]

Committed r143801: <http://trac.webkit.org/changeset/143801>