Fix margin size calculation for TableWrapper #13681

gpoesia · 2016-10-10T20:17:32Z

Fixes inline size calculation for TableWrapper. The table's width was always reaching the inline size equation solver as a specified variable, which was causing the system to be overdetermined when there was a margin specified for the table, and this caused the overflow reported in #12748. The fix consists in handling three cases when the table's width is not specified: if the preferred size of all columns fits, it is returned; if the minimum size does not fit, it is returned instead (it will overflow), otherwise, it is returned as a free variable (that should be solved together with the margin to some value above the minimum width and below the preferred width).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Wikipedia page wrong layout #12748
There are tests for these changes

This change is

stshine · 2016-10-10T23:48:03Z

@bors-servo try

bors-servo · 2016-10-10T23:48:06Z

⌛ Trying commit 0a58c02 with merge fdaa83b...

Fix margin size calculation for TableWrapper  Fixes margin size calculation for TableWrapper. The table's width was always reaching the inline size equation solver as a specified variable, which was causing the system to be overdetermined when there was a margin specified for the table, and this caused the overflow reported in #12748. The fix consists in letting the width be a free variable when it's not specified, so that the solver calculates it properly in the presence or absense of margins. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12748 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13681)

bors-servo · 2016-10-11T00:22:20Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-11T00:22:22Z

  ▶ FAIL [expected PASS] /_mozilla/css/absolute_table.html
  └   → /_mozilla/css/absolute_table.html ea33457f5a0be954e631e2bfebd8e73d5ea50566
/_mozilla/css/absolute_table_ref.html a040509b94135f47321dc6bacea999e81134f506
Testing ea33457f5a0be954e631e2bfebd8e73d5ea50566 == a040509b94135f47321dc6bacea999e81134f506

  ▶ FAIL [expected PASS] /_mozilla/css/table_center_a.html
  └   → /_mozilla/css/table_center_a.html 6b896b0d738a5413787db958265d6ff974922287
/_mozilla/css/table_center_ref.html 640c1955fd195c4c5b10fde093c74a5ab578720e
Testing 6b896b0d738a5413787db958265d6ff974922287 == 640c1955fd195c4c5b10fde093c74a5ab578720e

gpoesia · 2016-10-11T02:09:17Z

It seems there are more cases to handle, the tests are indeed broken. I'll take a look.

cbrewster · 2016-10-12T15:09:14Z

r? @pcwalton

jdm · 2016-10-25T14:36:35Z

@gpoesia Is it worth having someone else review the changes yet, or would you like to fix the failing tests first?

emilio · 2016-10-25T14:54:41Z

Note that a lot of the table code is being rewritten in #13870.

pcwalton · 2016-10-25T17:19:15Z

I think it'd be best to rebase this on top of #13870. I'll do that if you'd like.

gpoesia · 2016-10-31T12:31:11Z

@pcwalton Thank you, I think rebasing makes sense. By now that PR was already merged, right? I can just rebase on top of master then. I should be able to debug this this week.

jdm · 2016-10-31T13:29:33Z

Correct!

gpoesia · 2016-12-19T00:45:44Z

I've came back to this. After rebasing, the same situation persists: the test I added fails in master but not with this change; however, other related tests are broken. I'll look into these others.

The preferred size of all columns was always being used before, but this caused problems when it does not fit along with margins.

gpoesia · 2016-12-31T02:54:47Z

I've identified some more cases and updated the PR description to address them. The 2 test failures were fixed. I believe it is ready for review.

emilio · 2016-12-31T03:14:01Z

components/layout/table_wrapper.rs

@@ -778,7 +778,13 @@ fn initial_computed_inline_size(block: &mut BlockFlow,
                                                       containing_block_inline_size);
    match inline_size_from_style {
        MaybeAuto::Auto => {
-            MaybeAuto::Specified(min(containing_block_inline_size, preferred_width_of_all_columns))
+            if preferred_width_of_all_columns + table_border_padding <= containing_block_inline_size {


I thought that the idea of the preferred width is that nothing should be greater than it.

That being said, table layout is hard, and I'm probably not the best person to review this.

Oh, I know why this is correct now, whoops. Wasn't really inspired the day I wrote this.

emilio · 2016-12-31T03:14:47Z

r? @mbrubeck

jdm · 2017-01-19T21:56:19Z

@mbrubeck Review ping!

mbrubeck · 2017-01-23T15:43:42Z

@bors-servo r+

Thanks, and sorry for the delay!

bors-servo · 2017-01-23T15:43:43Z

📌 Commit d7a9c3f has been approved by mbrubeck

bors-servo · 2017-01-23T16:55:46Z

⌛ Testing commit d7a9c3f with merge b0f9119...

Fix margin size calculation for TableWrapper  Fixes inline size calculation for TableWrapper. The table's width was always reaching the inline size equation solver as a specified variable, which was causing the system to be overdetermined when there was a margin specified for the table, and this caused the overflow reported in #12748. The fix consists in handling three cases when the table's width is not specified: if the preferred size of all columns fits, it is returned; if the minimum size does not fit, it is returned instead (it will overflow), otherwise, it is returned as a free variable (that should be solved together with the margin to some value above the minimum width and below the preferred width). --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12748 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13681)

bors-servo · 2017-01-23T18:13:41Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned cbrewster Oct 10, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 10, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 11, 2016

highfive assigned pcwalton and unassigned cbrewster Oct 12, 2016

jdm added S-needs-rebase There are merge conflict errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 31, 2016

gpoesia force-pushed the master branch from 0a58c02 to 8014a5d Compare December 19, 2016 00:38

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 19, 2016

TableWrapper: fix content inline size calculation

d7a9c3f

The preferred size of all columns was always being used before, but this caused problems when it does not fit along with margins.

gpoesia force-pushed the master branch from 8014a5d to d7a9c3f Compare December 31, 2016 01:20

emilio reviewed Dec 31, 2016

View reviewed changes

highfive assigned mbrubeck and unassigned pcwalton Dec 31, 2016

KiChjang removed the S-needs-rebase There are merge conflict errors. label Jan 19, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 23, 2017

bors-servo merged commit d7a9c3f into servo:master Jan 23, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix margin size calculation for TableWrapper #13681

Fix margin size calculation for TableWrapper #13681

gpoesia commented Oct 10, 2016 •

edited

stshine commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 11, 2016

highfive commented Oct 11, 2016

gpoesia commented Oct 11, 2016

cbrewster commented Oct 12, 2016

jdm commented Oct 25, 2016

emilio commented Oct 25, 2016

pcwalton commented Oct 25, 2016

gpoesia commented Oct 31, 2016

jdm commented Oct 31, 2016

gpoesia commented Dec 19, 2016

gpoesia commented Dec 31, 2016

emilio Dec 31, 2016

emilio Jan 23, 2017

emilio commented Dec 31, 2016

jdm commented Jan 19, 2017

mbrubeck commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

Fix margin size calculation for TableWrapper #13681

Fix margin size calculation for TableWrapper #13681

Conversation

gpoesia commented Oct 10, 2016 • edited

stshine commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 11, 2016

highfive commented Oct 11, 2016

gpoesia commented Oct 11, 2016

cbrewster commented Oct 12, 2016

jdm commented Oct 25, 2016

emilio commented Oct 25, 2016

pcwalton commented Oct 25, 2016

gpoesia commented Oct 31, 2016

jdm commented Oct 31, 2016

gpoesia commented Dec 19, 2016

gpoesia commented Dec 31, 2016

emilio Dec 31, 2016

Choose a reason for hiding this comment

emilio Jan 23, 2017

Choose a reason for hiding this comment

emilio commented Dec 31, 2016

jdm commented Jan 19, 2017

mbrubeck commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

gpoesia commented Oct 10, 2016 •

edited