Handle row borders in border collapsing logic. #12437

gpoesia · 2016-07-13T17:25:00Z

Handle table row border when collapsing borders for a table row. The row border is combined with the cell's border using the already implemented conflict resolution logic.

This is a screenshot of the following test:

<!doctype html>
<html><body>
    <style>
      table {
        border-collapse: collapse;
      }
      tr {
        border: 1px solid black;
      }
    </style>
    <table>
      <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
    </table>
  </body>
</html>

The top border is missing, but I think that's a different bug, since it also does not show up when the border is in the cells, and not the rows. Also, when debugging the border collapsing structures, they seem ok (the top border seems to be there). I can look at that bug in a separate issue (or in this one too if you prefer).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11527 (github issue number if applicable).

These changes do not require tests because I didn't find how to automatically test it (will be happy to provide a test if there's infrastructure for this kind of test already in place).

Fixes #11527.

This change is

highfive · 2016-07-13T17:25:06Z

Warning

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

jdm · 2016-07-13T17:26:51Z

Layout code is usually tested using reference tests. We have documentation on creating them.

gpoesia · 2016-07-14T15:07:16Z

@jdm thanks! Sorry for missing that, I'll provide a test soon.
One doubt, though: since this PR still leaves one issue missing (the missing top border, which looks like it is caused by a different issue), what should the test look like? I guess I could investigate this other bug too, amend the PR and add a fully correct test.

jdm · 2016-07-14T15:49:53Z

That's a good question. Is there some way to reproduce the problem that doesn't trigger the other bug?

gpoesia · 2016-07-14T16:21:20Z

Actually there is, I can put borders in all rows except the topmost. Then with this PR the output is pretty much the same as the one Chrome and Firefox give:

<!doctype html>
<html><body>
    <style>
      table {
        border-collapse: collapse;
      }
      .b {
        border: 1px solid black;
      }
    </style>
    <table>
      <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr class="b"><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr class="b"><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
      <tr class="b"><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr>
    </table>
  </body>
</html>

Servo:

Chrome:

Firefox:

Servo from master:

I can make a ref test with this, then.

KiChjang · 2016-07-24T08:53:11Z

r? @pcwalton

pcwalton · 2016-07-25T21:59:05Z

@bors-servo: r+

bors-servo · 2016-07-25T21:59:06Z

📌 Commit e3d059e has been approved by pcwalton

bors-servo · 2016-07-25T23:44:28Z

⌛ Testing commit e3d059e with merge 10ad5c3...


        Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton

Handle row borders in border collapsing logic.  Handle table row border when collapsing borders for a table row. The row border is combined with the cell's border using the already implemented conflict resolution logic. This is a screenshot of the following test: ```html <!doctype html> <html><body> <style> table { border-collapse: collapse; } tr { border: 1px solid black; } </style> <table> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> </table> </body> </html> ``` <img src="https://dl.dropboxusercontent.com/u/10962672/Screenshots%20Servo/servo_tr_border_collapse.png"/> The top border is missing, but I think that's a different bug, since it also does not show up when the border is in the cells, and not the rows. Also, when debugging the border collapsing structures, they seem ok (the top border seems to be there). I can look at that bug in a separate issue (or in this one too if you prefer). ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11527 (github issue number if applicable).  - [X] These changes do not require tests because I didn't find how to automatically test it (will be happy to provide a test if there's infrastructure for this kind of test already in place).  Fixes #11527.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12437)

bors-servo · 2016-07-26T00:34:35Z

💔 Test failed - mac-rel-css

highfive · 2016-07-26T00:34:37Z

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

  ▶ PASS [expected FAIL] /css21_dev/html4/border-bottom-color-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-bottom-width-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-conflict-style-101.htm

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

  ▶ PASS [expected FAIL] /css21_dev/html4/border-left-color-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-left-width-applies-to-004.htm

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

  ▶ PASS [expected FAIL] /css21_dev/html4/border-right-color-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/border-right-width-applies-to-004.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/vertical-align-121.htm

pcwalton · 2016-07-26T00:38:54Z

Just needs a few test expectations updates. 11 new passes :)

gpoesia · 2016-07-28T01:23:13Z

Done :)

jdm · 2016-07-28T11:46:32Z

@bors-servo: r=pcwalton

bors-servo · 2016-07-28T12:58:48Z

⌛ Testing commit 9f9eb12 with merge 65853a7...


        Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton

Handle row borders in border collapsing logic.  Handle table row border when collapsing borders for a table row. The row border is combined with the cell's border using the already implemented conflict resolution logic. This is a screenshot of the following test: ```html <!doctype html> <html><body> <style> table { border-collapse: collapse; } tr { border: 1px solid black; } </style> <table> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> </table> </body> </html> ``` <img src="https://dl.dropboxusercontent.com/u/10962672/Screenshots%20Servo/servo_tr_border_collapse.png"/> The top border is missing, but I think that's a different bug, since it also does not show up when the border is in the cells, and not the rows. Also, when debugging the border collapsing structures, they seem ok (the top border seems to be there). I can look at that bug in a separate issue (or in this one too if you prefer). ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11527 (github issue number if applicable).  - [X] These changes do not require tests because I didn't find how to automatically test it (will be happy to provide a test if there's infrastructure for this kind of test already in place).  Fixes #11527.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12437)

bors-servo · 2016-07-28T13:56:17Z

💔 Test failed - linux-rel

jdm · 2016-07-28T14:54:52Z

This failure looks suspicious:

  ▶ FAIL [expected PASS] /_mozilla/css/border_collapse_simple_a.html
  └   → /_mozilla/css/border_collapse_simple_a.html e0dbf4c95e91bbc8314511a55f42ea1be552c274
/_mozilla/css/border_collapse_simple_ref.html c14395ed4b2c80aee21c9bd21c8b4b734fec07e5
Testing e0dbf4c95e91bbc8314511a55f42ea1be552c274 == c14395ed4b2c80aee21c9bd21c8b4b734fec07e5

The other one is #12580.

gpoesia · 2016-07-29T06:22:22Z

Hmm, @pcwalton do you know if the FIXME in that test is related to this PR?

<!DOCTYPE html>
<html>
<head>
<!--
    Tests that border collapse override and border collapse positioning works. All border widths
    are even numbers to avoid subpixel rounding issues (the handling of which is not spec'd).

    FIXME(pcwalton): This is currently offset by -2px in block and inline directions because we
    don't correctly handle collapsed borders when calculating `table_border_padding` in
    `table_wrapper.rs`.
-->
<link rel=match href=border_collapse_simple_ref.html>
<style>
html, body {
    margin: 0;
}
html {
    padding: 0;
}
body {
    /* See `FIXME` above. */
    padding: 2px;
}
...

It looks like the difference is indeed a small padding added by the PR. I'll take a look.

pcwalton · 2016-07-29T16:56:14Z

@gpoesia If you can fix the test by removing the 2px, then feel free to do that.

gpoesia · 2016-08-08T13:47:36Z

Came back from vacations and started looking deeper into this. There are some problems which are not related to the padding. The left border width (TableFlow::collapsed_inline_direction_border_widths_for_table[0]) is not calculated properly, and if I fix that manually for this test (in the code) the leftmost column width is slightly (and erroneously) increased. I have some clues and will keep debugging that the next few days.

gpoesia · 2016-08-10T16:49:56Z

I think I've fixed one of the issues, but a second one remains: the cells in the left column apparently have a slightly larger width than they should (2px). Still not sure why this is so.

As another note, I've noticed that even if I remove the 2px padding, the a and ref versions of the failing test do not match in Firefox either. It's visually noticeable if you open them in different tabs and switch: cell 0,0 in "ref" is slightly smaller (apparently by those same 2px) in both inline and block directions. I tried modifying the test so that they matched in Firefox, but then the cell 0,0 in servo with this PR is smaller (but only in the block direction; inline is fine).

@pcwalton: any idea of what might be going on?

pcwalton · 2016-08-17T16:30:20Z

If the test is failing in Firefox as well, feel free to just rewrite the test to test it some other way.

gpoesia · 2016-08-29T02:38:07Z

I've modified the test and it passes now, but it diverges slightly from Firefox (one cell's width in the block direction is 2px smaller; the widths in the inline direction are all identical, differently from the previous version of the test).

@pcwalton what do you think would be the most appropriate to do? I can keep debugging why the collapsing in the block direction is still a little different from Firefox. Do you have any tips on what I could look for?

pcwalton · 2016-08-29T21:31:02Z

Let's just file a FIXME and get this landed.


        Handle row borders in border collapsing logic.

Fixes #11527.

gpoesia · 2016-08-31T02:07:43Z

All right, just did that.

notriddle · 2016-08-31T16:28:15Z

@bors-servo r=pcwalton

bors-servo · 2016-08-31T16:28:16Z

📌 Commit a3af230 has been approved by pcwalton

bors-servo · 2016-08-31T16:28:19Z

⌛ Testing commit a3af230 with merge f5a546a...


        Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton

Handle row borders in border collapsing logic.  Handle table row border when collapsing borders for a table row. The row border is combined with the cell's border using the already implemented conflict resolution logic. This is a screenshot of the following test: ```html <!doctype html> <html><body> <style> table { border-collapse: collapse; } tr { border: 1px solid black; } </style> <table> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> <tr><td>Lorem</td><td>Ipsum</td><td>Sit</td><td>Dolor</td></tr> </table> </body> </html> ``` <img src="https://dl.dropboxusercontent.com/u/10962672/Screenshots%20Servo/servo_tr_border_collapse.png"/> The top border is missing, but I think that's a different bug, since it also does not show up when the border is in the cells, and not the rows. Also, when debugging the border collapsing structures, they seem ok (the top border seems to be there). I can look at that bug in a separate issue (or in this one too if you prefer). ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11527 (github issue number if applicable).  - [X] These changes do not require tests because I didn't find how to automatically test it (will be happy to provide a test if there's infrastructure for this kind of test already in place).  Fixes #11527.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12437)

bors-servo · 2016-08-31T17:24:44Z

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

highfive assigned KiChjang Jul 13, 2016

highfive added the S-awaiting-review label Jul 13, 2016

gpoesia force-pushed the gpoesia:tr_margin_fix branch from faa2b5c to e3d059e Jul 17, 2016

KiChjang closed this Jul 24, 2016

KiChjang reopened this Jul 24, 2016

highfive assigned pcwalton and unassigned KiChjang Jul 24, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 25, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 26, 2016

johannhof mentioned this pull request Jul 26, 2016

Hide border and padding on anonymous table objects #11783

Closed

4 of 4 tasks complete

gpoesia force-pushed the gpoesia:tr_margin_fix branch from e3d059e to 9f9eb12 Jul 28, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 28, 2016

highfive added the S-awaiting-merge label Jul 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 28, 2016

gpoesia force-pushed the gpoesia:tr_margin_fix branch from 9f9eb12 to 3b0aafe Aug 10, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Aug 10, 2016

gpoesia force-pushed the gpoesia:tr_margin_fix branch from 3b0aafe to bf105fa Aug 29, 2016

Handle row borders in border collapsing logic.

Loading status checks…

a3af230

Fixes #11527.

gpoesia force-pushed the gpoesia:tr_margin_fix branch from bf105fa to a3af230 Aug 31, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 31, 2016

bors-servo merged commit a3af230 into servo:master Aug 31, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 31, 2016

servo / servo

Handle row borders in border collapsing logic. #12437

Handle row borders in border collapsing logic. #12437

gpoesia commented Jul 13, 2016 •

edited by larsbergstrom

highfive commented Jul 13, 2016

jdm commented Jul 13, 2016

gpoesia commented Jul 14, 2016

jdm commented Jul 14, 2016

gpoesia commented Jul 14, 2016 •

edited

KiChjang commented Jul 24, 2016

pcwalton commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 26, 2016

highfive commented Jul 26, 2016

pcwalton commented Jul 26, 2016

gpoesia commented Jul 28, 2016

jdm commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

jdm commented Jul 28, 2016

gpoesia commented Jul 29, 2016

pcwalton commented Jul 29, 2016

gpoesia commented Aug 8, 2016

gpoesia commented Aug 10, 2016 •

edited by jdm

pcwalton commented Aug 17, 2016

gpoesia commented Aug 29, 2016 •

edited

pcwalton commented Aug 29, 2016

gpoesia commented Aug 31, 2016

notriddle commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

servo / servo

Join GitHub today

Handle row borders in border collapsing logic. #12437

Handle row borders in border collapsing logic. #12437

Conversation

gpoesia commented Jul 13, 2016 • edited by larsbergstrom

highfive commented Jul 13, 2016

jdm commented Jul 13, 2016

gpoesia commented Jul 14, 2016

jdm commented Jul 14, 2016

gpoesia commented Jul 14, 2016 • edited

KiChjang commented Jul 24, 2016

pcwalton commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 26, 2016

highfive commented Jul 26, 2016

pcwalton commented Jul 26, 2016

gpoesia commented Jul 28, 2016

jdm commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

jdm commented Jul 28, 2016

gpoesia commented Jul 29, 2016

pcwalton commented Jul 29, 2016

gpoesia commented Aug 8, 2016

gpoesia commented Aug 10, 2016 • edited by jdm

pcwalton commented Aug 17, 2016

gpoesia commented Aug 29, 2016 • edited

pcwalton commented Aug 29, 2016

gpoesia commented Aug 31, 2016

notriddle commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

gpoesia commented Jul 13, 2016 •

edited by larsbergstrom

gpoesia commented Jul 14, 2016 •

edited

gpoesia commented Aug 10, 2016 •

edited by jdm

gpoesia commented Aug 29, 2016 •

edited