Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fixed table layout column width distribution #16588

Merged

Conversation

@KiChjang
Copy link
Member

KiChjang commented Apr 24, 2017

Fixes #16324.

Replaces the incorrect CSS3 "distributing excess width to columns" algorithm and implements the simpler CSS2 fixed table layout algorithm.


This change is Reviewable

@highfive
Copy link

highfive commented Apr 24, 2017

Heads up! This PR modifies the following files:

  • @emilio: components/layout/table.rs
@highfive
Copy link

highfive commented Apr 24, 2017

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@KiChjang
Copy link
Member Author

KiChjang commented Apr 24, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 24, 2017

Trying commit 9e39a80 with merge 97623ce...

bors-servo added a commit that referenced this pull request Apr 24, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@emilio
Copy link
Member

emilio commented Apr 24, 2017

@highfive highfive assigned mbrubeck and unassigned asajeffrey Apr 24, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 24, 2017

@bors-servo clean retry

@bors-servo
Copy link
Contributor

bors-servo commented Apr 24, 2017

Trying commit 9e39a80 with merge fe256ad...

bors-servo added a commit that referenced this pull request Apr 24, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 24, 2017

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Apr 24, 2017

  ▶ FAIL [expected PASS] /css21_dev/html4/floats-149.htm
  ▶ FAIL [expected PASS] /_mozilla/css/fixed_percent.html
  ▶ FAIL [expected PASS] /_mozilla/css/border_spacing_fixed_layout_a.html
@KiChjang KiChjang force-pushed the KiChjang:fix-fixed-table-layout-column-width branch from 9e39a80 to 47ee799 Apr 25, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 25, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2017

Trying commit 47ee799 with merge d673aef...

bors-servo added a commit that referenced this pull request Apr 25, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2017

💔 Test failed - linux-rel-css

@KiChjang
Copy link
Member Author

KiChjang commented Apr 25, 2017

Tests with unexpected results:
  ▶ FAIL [expected PASS] /css21_dev/html4/background-attachment-applies-to-001.htm
  └   → /css21_dev/html4/background-attachment-applies-to-001.htm b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e
/css21_dev/html4/reference/background-attachment-applies-to-001-ref.htm da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16
Testing b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e == da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16

  ▶ FAIL [expected PASS] /css21_dev/html4/background-attachment-applies-to-003.htm
  └   → /css21_dev/html4/background-attachment-applies-to-003.htm b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e
/css21_dev/html4/reference/background-attachment-applies-to-001-ref.htm da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16
Testing b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e == da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16

  ▶ FAIL [expected PASS] /css21_dev/html4/background-attachment-applies-to-002.htm
  └   → /css21_dev/html4/background-attachment-applies-to-002.htm b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e
/css21_dev/html4/reference/background-attachment-applies-to-001-ref.htm da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16
Testing b3a9a6440147e1eb6ddb79f95cb5f68277f98e6e == da47ac50f6c835e9b6d4f2cbda6cc22a92e01f16

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-001a.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-001.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-005.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-002.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-003.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-006.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/float-applies-to-004a.htm

Iiiiiiiiiiinteresting results, going to investigate soon.

@KiChjang
Copy link
Member Author

KiChjang commented Apr 25, 2017

For all the test failures above, the flow tree indicates that the TableRowGroupFlow has a zero width. Tentatively guessing that something is wrong during assigning inline sizes.

@KiChjang KiChjang force-pushed the KiChjang:fix-fixed-table-layout-column-width branch from 47ee799 to bd47c4e Apr 26, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Apr 26, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 26, 2017

Trying commit bd47c4e with merge 05fe6cc...

bors-servo added a commit that referenced this pull request Apr 26, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Apr 28, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Apr 28, 2017

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

Trying commit 2d5833d with merge 4b5d587...

bors-servo added a commit that referenced this pull request Apr 28, 2017
…r=<try>

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Apr 28, 2017

{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, basic", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247476, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, inline-block", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247477, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, img in span", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247478, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, the don't-wrap rule is only for the purpose of calculating the width of the cell", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247479, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, the quirk shouldn't apply for <input>", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247482, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, non-auto width on cell", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247486, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, display:table-cell on span", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247488, "message": null, "expected": "FAIL"}
{"status": "PASS", "stack": null, "subtest": "The table cell width calculation quirk, display:table-cell on span, wbr", "action": "test_result", "test": "/quirks-mode/table-cell-width-calculation.html", "line": 247489, "message": null, "expected": "FAIL"}
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 28, 2017

Nice! r=mbrubeck with test expectations updated.

@jdm
Copy link
Member

jdm commented Apr 28, 2017

The tests expectations shouldn't be updated; it's an intermittent pass that #16640 hit as well.

@jdm
Copy link
Member

jdm commented Apr 28, 2017

@bors-servo: r=mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

📌 Commit 2d5833d has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

Testing commit 2d5833d with merge bf32a7b...

bors-servo added a commit that referenced this pull request Apr 28, 2017
…r=mbrubeck

Fix fixed table layout column width distribution

Fixes #16324.

Replaces the incorrect [CSS3 "distributing excess width to columns" algorithm](https://drafts.csswg.org/css-tables-3/#distributing-width-to-columns) and implements the simpler [CSS2 fixed table layout algorithm](https://drafts.csswg.org/css2/tables.html#fixed-table-layout).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16588)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

💔 Test failed - linux-rel-wpt

@jdm
Copy link
Member

jdm commented Apr 28, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

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

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

💔 Test failed - linux-rel-css

@jdm
Copy link
Member

jdm commented Apr 28, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

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

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2017

☀️ 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-msvc-dev
Approved by: mbrubeck
Pushing bf32a7b to master...

@bors-servo bors-servo merged commit 2d5833d into servo:master Apr 28, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@KiChjang KiChjang deleted the KiChjang:fix-fixed-table-layout-column-width branch Apr 28, 2017
@KiChjang KiChjang mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.