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

Wide tables will clip off the right side #111

Closed
CosmicHorrorDev opened this issue May 14, 2023 · 19 comments · Fixed by #129
Closed

Wide tables will clip off the right side #111

CosmicHorrorDev opened this issue May 14, 2023 · 19 comments · Fixed by #129
Labels
A-table Area: Dealing with markdown tables C-bug Category: Something isn't working

Comments

@CosmicHorrorDev
Copy link
Collaborator

Repro

| Header 1 | Header 2 | Header 3 |
| :---: | :---: | :---: |
| The quick brown fox jumps over the lazy dog | The quick brown fox jumps over the lazy dog | The quick brown fox jumps over the lazy dog |

Rendered by GitHub

Header 1 Header 2 Header 3
The quick brown fox jumps over the lazy dog The quick brown fox jumps over the lazy dog The quick brown fox jumps over the lazy dog

Rendered by inlyne

table_clipping.mp4
@CosmicHorrorDev CosmicHorrorDev added C-bug Category: Something isn't working A-table Area: Dealing with markdown tables labels May 14, 2023
@nicoburns
Copy link
Contributor

You could consider using Taffy's (https://github.com/DioxusLabs/taffy) CSS Grid support for laying out tables. It won't perfectly match the HTML table layout algorithm, but it ought to be pretty close and do a good job of sensibly distributing content to fit a space, and should also work well with nested tables.

@trimental
Copy link
Collaborator

@nicoburns I'll definitely look into this once the glyphon pr is merged. And also see if there's other places it can be used. Thanks

@nicoburns
Copy link
Contributor

@nicoburns I'll definitely look into this once the glyphon pr is merged. And also see if there's other places it can be used. Thanks

Awesome. You can probably use it for all your layout/positioning needs until you get down to the level of text/images (essentially for all the block-level HTML elements) if you want to. If you're willing to use Taffy's main branch then we just landed support for display: block which would probably make your life easier as that's the default display mode of most HTML elements (otherwise you could emulate this using a flexbox column). Some notes:

  • You may find this WIP high-level documentation helpful https://github.com/DioxusLabs/taffy/pull/444/files
  • The general strategy would be to be build a tree of nodes that is 1:1 with the structure of the HTML you are trying to render, setting styles like margin and padding to space thing properly.
  • For "leaf" nodes (i.e. text/images) taffy allows you provide a "measure function" (MeasureFunc) which is a boxed closure that takes a set of constraints (e.g. the available width) as parameters and expects you to return the size the element should take up.
  • For text nodes you would probably need to clone the cosmic-text buffer into the measure function so that it's layout method can be called on demand. We can add better support for borrowing data in measure functions if need be.
  • For images the measure function would be very simple and would just contain it's native size and aspect ratio.
  • Lists are just block nodes with a default margin
  • Tables would need to be lowered to CSS Grid. You'd need to count the number of columns (N) and then set grid_template_columns to repeat(N, auto), and you use the gap property to leave space for borders between cells. Nested tables would become nested grids (this is fully supported).
  • Details elements could make use of display: none

@trimental
Copy link
Collaborator

@nicoburns tried yesterday to convert the tables to a CSS grid but implemented it in a weird way due to not knowing about MeasureFunc. Thanks heaps for that. I'll try to get a semi working PR together soon and you can help guide it the rest of the way if your willing.

@trimental
Copy link
Collaborator

Also cloning the text for now won't be as dreadful as you'd think since the actual rendered text is kept cached, you'd just be cloning the actual string and styling (not great but not bad).

@nicoburns
Copy link
Contributor

Re: MeasureFunc. I've realised this isn't very well documented but:

  • The first parameter is "known_dimensions" (Size<Option<f32>>). If this is set in either dimension (width/height) then you should just return that value directly back out and only compute the other dimension (the value you return in this dimension will be ignored anyway as it already fixed as far as the layout algorithm is concerned). You may for example get called once with width: None, then again with a width set once the layout algorithm has determined width and is now trying to determine height.
  • The second parameter is "available_space" (Size<AvailableSpace>). In each dimension this can either be MinContent, MaxContent or Definite(Size<f32>). For text you can ignore the height property (unless you support vertical writing modes). For the width:
    • If you get MinContent you should return the size you get if take every possible wrapping opportunity (I think you can do this by setting cosmic-text buffer width to 0)
    • If you get MaxContent you should return the size you get if you don't wrap at all (I think you can do this by setting cosmic-text buffer width to i32::MAX)
    • If you get Definite then should return the size you get if you wrap at the given width (I think you can do this by setting cosmic-text buffer width to that size)

Where:

struct Size<T> { width: T, height: T }

Also cloning the text for now won't be as dreadful as you'd think since the actual rendered text is kept cached, you'd just be cloning the actual string and styling (not great but not bad).

Agreed. I figure that'll like be quick enough to start with, and it can be optimised later.

@trimental
Copy link
Collaborator

@nicoburns I'm currently dealing with a bug where the spacing between grid items seems to grow relative to the size of the root node, any suggestions?

@nicoburns
Copy link
Contributor

@trimental Are you using a percentage size for the gap property? Otherwise it would be helpful to see your code and/or what styles you are using for each node.

@trimental
Copy link
Collaborator

Heres the gist of the taffy code https://gist.github.com/trimental/0e2dcf9eeca78cd69eb9d6d077bf2717. The width of each node seems to be calculated correctly, its just the position of each node except the first per axis that grows. (Btw that code is very wip. I turned off gap to see if that fixed anything)

Correctish:
image

Incorrect:
image

@nicoburns
Copy link
Contributor

nicoburns commented May 24, 2023

I think the issue here is that you're setting the height of the grid to bounds.0 but you should be setting it to bounds.1. Alternatively if you want a content-based height then you should just set the height to auto. You could also optionally set the width to auto (or just not size width/height at all as they default to auto). I suspect the best option might be to omit the size and max_size properties from the grid styles entirely, and pass in the bounds to compute_layout where you are currently passing in TaffyMaxContent::MAX_CONTENT.

@nicoburns
Copy link
Contributor

grid_template_columns: vec![repeat(taffy::style::GridTrackRepetition::Count(max_columns as u16), vec![auto(); max_columns]); self.headers.len() + self.rows.len()]

You're also generating far too many columns here. You either want:

grid_template_columns: vec![repeat(max_columns as u16, vec![auto()])]
or
grid_template_columns: vec![auto(); max_columns]

For rows you want:

grid_template_rows: vec![repeat((self.headers.len() + self.rows.len()) as u16, vec![auto()])]
or
grid_template_rows: vec![auto(); self.headers.len() + self.rows.len()]

or you can simply omit the grid_template_rows style and extra rows will be generated as necesary. In fact, as you're explicitly setting grid_row and grid_column on each table cell, that's also true of grid_template_columns. The algorithm is smart enough to compute max_columns itself.

@trimental
Copy link
Collaborator

@nicoburns I'm a bit confused with passing the bounds as available_space. Won't it just get turned into max_content? https://github.com/DioxusLabs/taffy/blob/5443996198f02cf5a6c6493499df3cf08394a70f/src/compute/grid/mod.rs#L168

@nicoburns
Copy link
Contributor

nicoburns commented May 24, 2023

@nicoburns I'm a bit confused with passing the bounds as available_space. Won't it just get turned into max_content? https://github.com/DioxusLabs/taffy/blob/5443996198f02cf5a6c6493499df3cf08394a70f/src/compute/grid/mod.rs#L168

So each node in the tree gets a different "available space" passed to it. You get to pass in the "available space" of the root node (in this case the grid). And that node's algorithm will choose an "available space" to pass into it's children.

The line you are referencing here is the grid container computing what it will pass as available space when sizing it's children. But the grid algorithm will measure it's children multiple times with different constraints and intelligently interpolate between their "min content" and "max context" sizes (according to it's own "available space", the sizes given to tracks (rows/columns), and the sizes of other grid children, etc) so that doesn't really imply anything about the sizes that nodes will end up.

If you pass in definite available space:

let available_space = Size {
     width: AvailableSpace::Definite(bounds.0),
     height: AvailableSpace::Definite(bounds.1),
};
taffy.compute_layout(root, available_space).unwrap();

Or perhaps only constraining the width, leaving the height indefinite because we can scroll:

let available_space = Size {
     width: AvailableSpace::Definite(bounds.0),
     height: AvailableSpace::MaxContent,
};
taffy.compute_layout(root, available_space).unwrap();

then the grid will max out at that width (wrapping text as needed) unless it contains content that can't be shrunk (for example a really long word with no line-break opportunities).

@trimental
Copy link
Collaborator

I've put the code I've got at the moment in this branch with this commit c109c60. I'm guessing that I must be either constructing the tree wrong or handling the measure function wrong. Setting the node width length to bounds.0 seems to make it resize but just using available_size works (with a weird aligning issue though).

@nicoburns
Copy link
Contributor

@trimental What exactly is going wrong here? This commit seems to render the table in the Taffy README just fine (well, except that the "Yoga" column wraps, which it shouldn't):

Screenshot 2023-05-24 at 20 47 28

I have simplified the text measure func (and updated it to also use the known_dimensions as bound when set) here: https://github.com/nicoburns/inlyne/tree/taffy-table-1

Setting the node width length to bounds.0 seems to make it resize

This makes sense. By setting the width style you are forcing the table to take up at least that size, not just setting it as a bound (and CSS grid defaults to stretch alignment, so by default it will stretch the columns to fit that space)

@nicoburns
Copy link
Contributor

I think you might want to revert back to setting grid_row and grid_column on each individual table cell. This will work better than the current setup in the case that there is a row with missing cells.

@trimental
Copy link
Collaborator

@nicoburns if possible id like to mimic the vscode markdown viewer by wrapping lines if the window size isn't large enough to display the table. Although it does look like wrapping is occuring in your example.

@nicoburns
Copy link
Contributor

Ah, I didn't realise it wasn't wrapping properly. I think it's a bug in Taffy that the Grid doesn't properly constrain itself by the available_space parameter (or max_width) when it's the root node. Finite "available_space" on the root node is actually something that our test harness isn't setup to test currently, so I'm not entirely suprised this was missed.

I will try to get that fixed (or at least investigated), but in the meantime I was able to workaround this issue by wrapping the grid in a flexbox node. I've pushed a few commits to the same branch as before:

  • The flexbox wrapping fix
  • Add a table with wrapping content to the temp.md example
  • Replace the unstable feature you enabled on your branch with a small utility function that can be used on stable

(main...nicoburns:inlyne:taffy-table-1)

@trimental
Copy link
Collaborator

Thanks. I really appreciate the time and knowledge your providing. Once this is in I'll start looking into the other areas we can use taffy.

@CosmicHorrorDev CosmicHorrorDev linked a pull request May 26, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-table Area: Dealing with markdown tables C-bug Category: Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants