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 Typst table wrapping to prevent font family and size spill #11113

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

younes-io
Copy link
Contributor

Fixes #11093

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 22, 2024

Thanks for your contribution, @younes-io!

As you can see, this fix causes infinite recursion and many of the tests time out. I think this is because it substitutes the table element with a Div containing the table, then recurses in and again tries to convert the table.

I can reproduce this locally by checking out your branch and then attempting to e.g.

quarto render tests/docs/smoke-all/typst/gt-islands.qmd

If this is the correct root cause, you could try testing if the table already has the class typst:no-figure and not do the substitution in that case. (There may be a more elegant solution, but this is what comes quickly to mind.)

@cscheid
Copy link
Collaborator

cscheid commented Oct 22, 2024

If this is the correct root cause, you could try testing if the table already has the class typst:no-figure and not do the substitution in that case. (There may be a more elegant solution, but this is what comes quickly to mind.)

FWIW, I think this still doesn't work on topdown traversals.

Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the return foo, false pattern, we don't need the typst-processed class, right?

@gordonwoodhull gordonwoodhull force-pushed the fix-typst-table-wrapping branch from 316a70a to 6999e08 Compare October 23, 2024 15:21
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 23, 2024

I've squashed these commits and moved the fix the specific place and condition where it is needed, and added a test.

As noted in #11093, this is a temporary change until we get the fix into Pandoc, and a couple of tests were failing due to unneeded changes to the Typst output. I'd rather avoid the churn of changing the tests twice.

Let's see if this passes now!

@gordonwoodhull
Copy link
Contributor

Merging. Thanks @younes-io!

@gordonwoodhull gordonwoodhull merged commit 61387d2 into quarto-dev:main Oct 23, 2024
47 checks passed
@younes-io
Copy link
Contributor Author

Thank you @gordonwoodhull for your help & the fix!

BTW, I was trying to set up a .devcontainer to make sure I could run tests in a clean environment, but ran into lots of errors during setup... After a couple hours, I had to put it aside. I might pick it up another day! Was wondering if you could share any docs or links (beyond what's in the README) that would help set up a dev environment quickly - without all the hassle of debugging dependencies and version requirements.

Thank you again! And thanks to @cscheid for being so patient as well! 😄

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.

Font family and size can spill out of Typst raw html blocks
3 participants