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 crash with certain right-aligned text #10271

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Oct 26, 2023

Objective

Fixes #9395
Alternative to #9415 (See discussion here)

Solution

Do clamping like fit-content.

Notes

I am not sure if this is a valid approach. It doesn't seem to cause any obvious issues with our existing examples.

@rparrett rparrett requested a review from nicoburns October 26, 2023 14:56
@rparrett rparrett added C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash A-UI Graphical user interfaces, styles, layouts, and widgets labels Oct 26, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

If nothing else, this seems like a safe way to avoid the panic.

I think it may actually be clearer to just do the clamp logic manually, but I won't block on that.

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Taffy (following how CSS deals with min-width > max-width) does val.min(max).max(min) instead of val.clamp(min, max), which avoids the panic introduced by the stdlib clamp by giving min priority over max. Perhaps we could also do that here?

See: DioxusLabs/taffy#261

@nicoburns
Copy link
Contributor

Hmm... Actually the spec (https://www.w3.org/TR/css-sizing-3/#column-sizing) defines a fit-content size as:

min(max-content size, max(min-content size, limit))

So I think that would be :

x.max(self.info.min.x).min(self.info.max.x)

in the function above

@rparrett
Copy link
Contributor Author

Okay, thanks for pointing out that spec.

I'll try that out and test with the problematic right aligned text constrained in various ways and see if I can find a difference between us/browsers there just to be sure.

@nicoburns
Copy link
Contributor

This still ultimately seems like a text layout / measurement bug to me. But we should probably do something along the lines of this PR anyway so that we at least don't panic when text layout is returning something nonsensical.

From alexheretic/glyph-brush#167:

There are other cases where glyphs may actually escape these bounds, like a small (or zero) width bound and a larger single word. It is up to the renderer to decide how to handle this, cutting off the glyph as it goes oob or just drawing it oob.

Seems to me that collapsible whitespace ought not to count towards the min-content size. Whereas I would expect a single very long alphanumeric word to extend that size. Not sure if we can implement that.

@rparrett
Copy link
Contributor Author

rparrett commented Oct 26, 2023

I swapped to nicoburns' suggestion of x.max(self.info.min.x).min(self.info.max.x) here.

In my testing I just became less confident that I knew what was going on. It is hard for me to say which bits of the css spec actually apply here. (the "auto box sizes fit-content" section seems to suggest doing the exact opposite thing). I haven't yet figured out what a proper comparison with a browser would even be.

But this should do nothing except in the cases where we would crash previously, so I guess that's an improvement.

@nicoburns
Copy link
Contributor

The CSS spec may be assuming that min-content-size < max-content-size (in which case it doesn't matter which way around you apply the operations). Because it fundamentally doesn't make sense that the size in the "inline axis" (so horizontal axis for horizontal text) when soft-wrapping is allowed would ever be larger than the size when soft-wrapping is not allowed (modulo all other inputs being the same).

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 26, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 26, 2023
Merged via the queue into bevyengine:main with commit befbf52 Oct 26, 2023
22 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

Fixes bevyengine#9395
Alternative to bevyengine#9415 (See discussion here)

## Solution

Do clamping like
[`fit-content`](https://www.w3.org/TR/css-sizing-3/#column-sizing).

## Notes

I am not sure if this is a valid approach. It doesn't seem to cause any
obvious issues with our existing examples.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Fixes bevyengine#9395
Alternative to bevyengine#9415 (See discussion here)

## Solution

Do clamping like
[`fit-content`](https://www.w3.org/TR/css-sizing-3/#column-sizing).

## Notes

I am not sure if this is a valid approach. It doesn't seem to cause any
obvious issues with our existing examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

right-aligned text panic
3 participants