Skip to content

/ servo

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

Jump to bottom

<tr> border is not displayed #11527

Closed
ocerman opened this issue May 31, 2016 · 5 comments
Closed

<tr> border is not displayed #11527

ocerman opened this issue May 31, 2016 · 5 comments
Labels
A-layout/table C-assigned C-has-manual-testcase

Comments

@ocerman
Copy link

@ocerman ocerman commented May 31, 2016 •
edited by KiChjang

tborder

<!doctype html>
<html><body>
    <style>
        table{
            border-collapse: collapse;
        }
        tr{
            border: 1px solid black;
        }
    </style>
    <table>
        <tr><td>Lorem</td></tr>
        <tr><td>ipsum</td></tr>
    </table>
</body></html>
@gpoesia
Copy link
Contributor

@gpoesia gpoesia commented Jul 4, 2016

I'm taking a look at this.
@jdm
Copy link
Member

@jdm jdm commented Jul 4, 2016

@gpoesia Great! Please ask questions about anything that's unclear!
@jdm jdm added the C-assigned label Jul 4, 2016
@gpoesia
Copy link
Contributor

@gpoesia gpoesia commented Jul 5, 2016

@jdm Alright, thanks! I managed to reproduce it and found the code in components/layout/table_row.rs which handles the "collapse" logic (if border-collapse is not "collapse" it works). Will keep investigating and ask if I get stuck :)
@metajack
Copy link
Contributor

@metajack metajack commented Jul 5, 2016

Border collapse is pretty difficult to figure out. You will probably want to have both Chrome and Firefox involved in testing, since the behavior across mainstream browsers does not always agree.

@pcwalton wrote the border collapse code, so if you have specific questions feel free to ping him.
@gpoesia
Copy link
Contributor

@gpoesia gpoesia commented Jul 10, 2016

Just giving some update on this. I found out that in the end of TableFlow::bubble_inline_sizes, all inline and block_{start,end} borders have their style set to none. If I manually set them to solid (all block_start, block_end, and the first inline borders, which correspond to the left and right extremities,) it works, so I assume the bug is indeed during the flow tree construction.

I think it's related to #8586, and the fix would be to correctly implement CollapsedBorderProvenance::FromTableRow, so that row borders descend to <td>s. If the borders are placed in <td>'s instead, it mostly works (aside from the left and upper edges of the table: that may be related, but collapsing inside the table works). @pcwalton does this make sense to you? Any tips on this from the top of your head? I'll try that this week.
gpoesia added a commit to gpoesia/servo that referenced this issue Jul 13, 2016
@gpoesia
Handle row borders in border collapsing logic.
faa2b5c 
Fixes servo#11527.
@gpoesia gpoesia mentioned this issue Jul 13, 2016
Merged
4 of 4 tasks complete
gpoesia added a commit to gpoesia/servo that referenced this issue Jul 17, 2016
@gpoesia
Handle row borders in border collapsing logic.
e3d059e 
Fixes servo#11527.
bors-servo added a commit that referenced this issue Jul 25, 2016
@bors-servo
Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton
Loading status checks…
10ad5c3 
Handle row borders in border collapsing logic.

<!-- Please describe your changes on the following line: -->
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).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [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).

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #11527.

<!-- 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/12437)
<!-- Reviewable:end -->
gpoesia added a commit to gpoesia/servo that referenced this issue Jul 28, 2016
@gpoesia
Handle row borders in border collapsing logic.
9f9eb12 
Fixes servo#11527.
bors-servo added a commit that referenced this issue Jul 28, 2016
@bors-servo
Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton
Loading status checks…
65853a7 
Handle row borders in border collapsing logic.

<!-- Please describe your changes on the following line: -->
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).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [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).

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #11527.

<!-- 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/12437)
<!-- Reviewable:end -->
gpoesia added a commit to gpoesia/servo that referenced this issue Aug 10, 2016
@gpoesia
Handle row borders in border collapsing logic.
3b0aafe 
Fixes servo#11527.
gpoesia added a commit to gpoesia/servo that referenced this issue Aug 29, 2016
@gpoesia
Handle row borders in border collapsing logic.
bf105fa 
Fixes servo#11527.
gpoesia added a commit to gpoesia/servo that referenced this issue Aug 31, 2016
@gpoesia
Handle row borders in border collapsing logic.
a3af230 
Fixes servo#11527.
bors-servo added a commit that referenced this issue Aug 31, 2016
@bors-servo
Auto merge of #12437 - gpoesia:tr_margin_fix, r=pcwalton
Loading status checks…
f5a546a 
Handle row borders in border collapsing logic.

<!-- Please describe your changes on the following line: -->
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).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [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).

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Fixes #11527.

<!-- 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/12437)
<!-- Reviewable:end -->
samuknet added a commit to samuknet/servo that referenced this issue Sep 6, 2016
@gpoesia @samuknet
Handle row borders in border collapsing logic.
6005a5e 
Fixes servo#11527.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-layout/table C-assigned C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Handle row borders in border collapsing logic.
4 participants
@jdm @metajack @gpoesia @ocerman
You can’t perform that action at this time.