Account for percentages in fixed table layout #13192

notriddle · 2016-09-07T05:12:09Z

Don't just use the minimum length all the time.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13166 (github issue number if applicable).
There are tests for these changes

This change is

highfive · 2016-09-07T05:12:13Z

Warning

These commits modify layout code, but no tests are modified. Please consider adding a test!

pcwalton · 2016-09-07T05:30:22Z

Could you add a reftest for this? Thanks!

notriddle · 2016-09-07T05:39:31Z

@pcwalton Done.

emilio · 2016-09-07T07:38:44Z

@bors-servo: r=pcwalton,emilio

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

components/layout/table.rs, line 376 [r1] (raw file):

                        });
                    }
                } else if num_unspecified_inline_sizes != 0 {

Could you convert this branch in an else if there are any other reftests to update?

Comments from Reviewable

bors-servo · 2016-09-07T07:38:45Z

📌 Commit ff5aa87 has been approved by pcwalton,emilio

bors-servo · 2016-09-07T07:39:20Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #13190

bors-servo · 2016-09-07T07:39:20Z

📌 Commit ff5aa87 has been approved by pcwalton,emilio

bors-servo · 2016-09-07T08:15:58Z

⌛ Testing commit ff5aa87 with merge 1815802...


        Auto merge of #13192 - notriddle:master, r=pcwalton,emilio

Account for percentages in fixed table layout Don't just use the minimum length all the time. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13166 (github issue number if applicable). - [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/13192)

bors-servo · 2016-09-07T09:17:35Z

💔 Test failed - mac-rel-wpt

highfive · 2016-09-07T09:17:37Z

  ▶ TIMEOUT [expected OK] /webgl/conformance-1.0.3/conformance/textures/origin-clean-conformance.html

Ms2ger · 2016-09-07T12:30:49Z

Is there a spec for this?

notriddle · 2016-09-07T15:50:28Z

I found the applicable spec.

notriddle · 2016-09-07T16:28:46Z

I don't think this is actually correct, either (though it does fix Github and my testcase is correct). The actual rules for assigning column widths in fixed mode are this:

These rules [the rules for distributing excess width in automatic layout mode] do not apply when the table is laid out in fixed mode. In this case, the simpler rules that follow apply instead:

If there are any columns with no width specified, the excess width is distributed in equally to such columns

otherwise, if there are columns with non-zero length widths from the base assignment, the excess width is distributed proportionally to width among those columns

otherwise, if there are columns with non-zero percentage widths from the base assignment, the excess width is distributed proportionally to percentage width among those columns

otherwise, the excess width is distributed equally to the zero-sized columns

In other words, we need to count the number of columns with percentage widths, and follow step 2 if they're not zero.


        Account for percentages in fixed table layout

Fixes #13166

notriddle · 2016-09-07T16:43:58Z

There, thanks for asking for a spec reference.


        Add reftest for #13166

notriddle · 2016-09-08T05:17:45Z

@bors-servo try


        Auto merge of #13192 - notriddle:master, r=<try>

Account for percentages in fixed table layout Don't just use the minimum length all the time. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13166 (github issue number if applicable). - [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/13192)

bors-servo · 2016-09-08T05:17:48Z

⌛ Trying commit 516adfa with merge 2f8ba13...

bors-servo · 2016-09-08T06:46:35Z

💔 Test failed - linux-rel

highfive · 2016-09-08T06:46:37Z

  ▶ PASS [expected FAIL] /css21_dev/html4/floats-149.htm


        Change test expectation: floats-149 = PASS

pcwalton · 2016-09-09T03:49:58Z

@bors-servo: r+

bors-servo · 2016-09-09T03:49:59Z

📌 Commit 102ea5a has been approved by pcwalton

bors-servo · 2016-09-09T04:17:07Z

⌛ Testing commit 102ea5a with merge 2d13178...


        Auto merge of #13192 - notriddle:master, r=pcwalton

Account for percentages in fixed table layout Don't just use the minimum length all the time. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13166 (github issue number if applicable). - [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/13192)

bors-servo · 2016-09-09T05:08:26Z

💔 Test failed - mac-rel-wpt

highfive · 2016-09-09T05:08:28Z

  ▶ ERROR [expected OK] /webgl/conformance-1.0.3/conformance/ogles/GL/build/build_009_to_016.html
  └   → gl.getProgramInfoLog is not a function

notriddle · 2016-09-09T05:24:34Z

@bors-servo retry

#13165

bors-servo · 2016-09-09T06:09:08Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-09-09T07:00:10Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned jdm Sep 7, 2016

highfive added the S-awaiting-review label Sep 7, 2016

highfive assigned pcwalton and unassigned jdm Sep 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 7, 2016

Account for percentages in fixed table layout

070bb7f

Fixes #13166

highfive added S-awaiting-review and removed S-tests-failed labels Sep 7, 2016

Add reftest for #13166

Loading status checks…

516adfa

highfive added the S-tests-failed label Sep 8, 2016

Change test expectation: floats-149 = PASS

Loading status checks…

102ea5a

highfive removed the S-tests-failed label Sep 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 9, 2016

bors-servo merged commit 102ea5a into servo:master Sep 9, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Account for percentages in fixed table layout #13192

Account for percentages in fixed table layout #13192

notriddle commented Sep 7, 2016 •

edited by larsbergstrom

highfive commented Sep 7, 2016

pcwalton commented Sep 7, 2016

notriddle commented Sep 7, 2016

emilio commented Sep 7, 2016 •

edited

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

highfive commented Sep 7, 2016

Ms2ger commented Sep 7, 2016

notriddle commented Sep 7, 2016 •

edited

notriddle commented Sep 7, 2016

notriddle commented Sep 7, 2016 •

edited

notriddle commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

highfive commented Sep 8, 2016

pcwalton commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

highfive commented Sep 9, 2016

notriddle commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

servo / servo

Join GitHub today

Account for percentages in fixed table layout #13192

Account for percentages in fixed table layout #13192

Conversation

notriddle commented Sep 7, 2016 • edited by larsbergstrom

highfive commented Sep 7, 2016

pcwalton commented Sep 7, 2016

notriddle commented Sep 7, 2016

emilio commented Sep 7, 2016 • edited

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

bors-servo commented Sep 7, 2016

highfive commented Sep 7, 2016

Ms2ger commented Sep 7, 2016

notriddle commented Sep 7, 2016 • edited

notriddle commented Sep 7, 2016

notriddle commented Sep 7, 2016 • edited

notriddle commented Sep 8, 2016

bors-servo commented Sep 8, 2016

bors-servo commented Sep 8, 2016

highfive commented Sep 8, 2016

pcwalton commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

highfive commented Sep 9, 2016

notriddle commented Sep 9, 2016

bors-servo commented Sep 9, 2016

bors-servo commented Sep 9, 2016

notriddle commented Sep 7, 2016 •

edited by larsbergstrom

emilio commented Sep 7, 2016 •

edited

notriddle commented Sep 7, 2016 •

edited

notriddle commented Sep 7, 2016 •

edited