asciidoctor / asciidoctor-stylesheet-factory

!DEPRECATED! This was the utility project for producing the default stylesheet for the HTML converter in Asciidoctor. The source of the default stylesheet now lives in the main Asciidoctor repository.
Other
175 stars 87 forks source link

Use cell borders instead of table borders #42

Closed ggrossetie closed 3 years ago

ggrossetie commented 4 years ago

Resolves https://github.com/asciidoctor/asciidoctor/issues/3387

mojavelinux commented 4 years ago

The problem here is that this is notoriously difficult to test manually. It took me many days to get what we have working. Ideally, we'd have a visual test (in core).

I'm also not sure we can achieve all the permutations using border collapsing. Here's a list:

Then we also have to test with and without a table header and with and without a table footer.

(And then there are the rowspans and colspans, which we don't full support as it is).

mojavelinux commented 4 years ago

What we could do is put in an optimization for the default case, which is frame=all grid=all.

table.frame-all.grid-all{border-collapse:collapse;border-width:0}
table.frame-all.grid-all>*>tr:nth-child(n)>.tableblock{border-width:1px}

However, this doesn't seem to fix it, even though it should.

mojavelinux commented 4 years ago

^ I can't understand why that patch doesn't fix the problem. I still get a missing border at the top of the page where the table breaks. Do you see a different result?

mojavelinux commented 4 years ago

Here's the complete CSS to cover the cases when we can use border collapsing:

table.frame-all.grid-all,table.frame-ends.grid-rows,table.frame-sides.grid-cols{border-collapse:collapse;border:0}
table.frame-all.grid-all>*>tr:nth-child(n)>.tableblock{border-width:1px}
table.frame-ends.grid-rows>*>tr:nth-child(n)>.tableblock{border-width:1px 0}
table.frame-sides.grid-cols>*>tr:nth-child(n)>.tableblock{border-width:0 1px}

I'm trying to think how we could eliminate the need for the tr:nth-child(n) specifier, but every other combination I try results in more lines.

mojavelinux commented 3 years ago

This patch does not cover all the cases, but I can see where you're going (using cell borders instead of table borders, where possible). I decided to run with it to see how far I could get. I think I managed to get it fully working. It even works with the original case in Asciidoctor Web PDF that led to this issue.

There's one decision to make. In order to properly support all permutations with the frame on (all, sides, ends) and the grid off in various directions (none, cols, rows), we still need the border on the table. These end up serving as fallback borders.

Instead of doing it that way, we could selectively apply the border to the cell to cover these cases. However, unless we determine that adding the border to the table itself as a fallback is problematic, I'd rather use it because it's simpler and more accurate.

The reason relying only on cell borders alone is problematic is because of rowspan and colspan. In these cases, a cell can extend into a row or column it's not in as reflected in the HTML. As a result, our selectors can miss it. This is acceptable for the grid, but it's better not to rely on it for the frame.

mojavelinux commented 3 years ago

The reason relying only on cell borders alone is problematic is because of rowspan and colspan.

We could address this in the semantic HTML5 converter by marking these cells if they reach the boundary of a table. I'll note that in the issue.

mojavelinux commented 3 years ago

I have submitted an alternate PR (#49) that covers all the cases described in https://github.com/asciidoctor/asciidoctor/issues/3387#issuecomment-717816343.

I also put the selectors for drawing the frame border using table cells in a comment. However, since it suffers from the rowspan/colspan problem, I don't think we should rely on it.