Skip to content

Render basic tables and detect them in rules#507

Open
masonium wants to merge 11 commits intodaisy:mainfrom
orchid-initiative:array_rule
Open

Render basic tables and detect them in rules#507
masonium wants to merge 11 commits intodaisy:mainfrom
orchid-initiative:array_rule

Conversation

@masonium
Copy link
Contributor

Addresses issue #283 .

This PR adds some internal functions to compute table dimensions (rows and columns) taking rowspans and colspans into account, and uses them to render tables with the :array intent as "table with m rows and n columns".

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

@rileeki
Copy link

rileeki commented Feb 28, 2026

@Jeffrey-Kuan

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

Any thoughts?

@rileeki
Copy link

rileeki commented Feb 28, 2026

Also, just a note: this PR currently fails one of the CI checks, but so does the main branch. @masonium submitted a separate PR (#506) to fix the test that's failing.

@NSoiffer
Copy link
Collaborator

NSoiffer commented Mar 1, 2026

Using frame, etc., is a good idea and will work for legacy MathML so we should keep it. However, in MathML core (which is the future), mtable are supposed to be styled with CSS so that info won't likely be around unless it is a local CSS style (style="...."). If you want, you could add a check for that. But people will likely use a class for that so there is no hope to get that info in general.

One option is to look at the parent of the table and see if has delimiters around it (parens, brackets, vertical bars, double vertical bars, ???).

Comments on the code:
speech.rs:

  • why add std::format::Debug and pub trait TreeOrString<'c, 'm:'c, T: Debug> : Debug {?

xpath-functions.rs:

  • why is struct CountTableDims pub?
  • at least when looking in github's PR files changed tab, the indenting is messed up making it hard to read the code.
  • the spanning column code only looks a colspan in the first row. If you are supporting colspan, shouldn't you get this right no matter where the colspan is located. It isn't easy. That's Handle rowspan/columnspan #33.
  • rowspan isn't handled at all. No need for CountTableRows if you're not handling these.

mtable.rs:

  • there aren't any tests that use rowspan or colspan. It is good that you have a unit test in xpath-functions.rs, but you need to test the actual speech also.

rowspan and colspan/columnspan are fully handled as dictated by MathML
descriptions. Empty rows and cells are properly accounted for.
@masonium
Copy link
Contributor Author

masonium commented Mar 2, 2026

I'll try to address everything. For code-specific comments, it's probably easier for both of us to use the commenting feature on Github (pressing the + next to the line numbers on the diffs) so that it's a bit easier to track individual threads.

Using frame, etc., is a good idea and will work for legacy MathML so we should keep it. However, in MathML core (which is the future), mtable are supposed to be styled with CSS so that info won't likely be around unless it is a local CSS style (style="...."). If you want, you could add a check for that. But people will likely use a class for that so there is no hope to get that info in general.

One option is to look at the parent of the table and see if has delimiters around it (parens, brackets, vertical bars, double vertical bars, ???).

I'll play around with looking for delimiters.

Comments on the code: speech.rs:

* why add `std::format::Debug` and `pub trait TreeOrString<'c, 'm:'c, T: Debug> : Debug {`?

I wanted to debug some of the functions that take a T: TreeOrString using println!("{:?}", ...). Since TreeOrString is meant to be a closed trait, I don't think this hinders future implementation.

xpath-functions.rs:

* why is `struct CountTableDims` pub?

It shouldn't be. Fixed.

* at least when looking in github's PR files changed tab, the indenting is messed up making it hard to read the code.

Fixed. For what it's worth, the existing code also contains some mix of tabs and spaces (see fn test_is_simple in xpath_functions.rs, for instance), causing rendering issues in e.g. Github. My editor usually does a good job of matching what's already in the file, but it's not foolproof. Another benefit of #273 .

* the spanning column code only looks a `colspan` in the first row. If you are supporting `colspan`, shouldn't you get this right no matter where the `colspan` is located. It isn't easy. That's [Handle rowspan/columnspan #33](https://github.com/daisy/MathCAT/issues/33).

* `rowspan` isn't handled at all. No need for `CountTableRows` if you're not handling these.

I adjusted the code so it does a more thorough accounting here, now that I've read the MathML spec and have a reasonable handle on this.

One caveat is that, though the spec doesn't seem to fully address it, extra rows that are implied by a dangling rowspan (for instance, an <mtd rowspan="2"> in the last row) does not seem to rendered, so I'm not counting them towards the row total. If they were, then the CountTableRows function would be necessary. If we agree on assuming that behavior, I can get rid of CountTableRows.

The column count is more explicitly defined as the maximum across all rows.

mtable.rs:

* there aren't any tests that use rowspan or colspan. It is good that you have a unit test in xpath-functions.rs, but you need to test the actual speech also.

I still need to add these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants