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

UI text wrapping and LineBreakOn example #7761

Merged
merged 9 commits into from
Apr 24, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 20, 2023

Objective

An example demonstrating more text layout options, text wrapping and LineBreakOn.

text_wrap

Won't look exactly like this on main because of the remaining bugs in text.

@ickshonpe ickshonpe changed the title Added UI text wrapping example UI text wrapping and LineBreakOn example Feb 20, 2023
@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

1 similar comment
@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Examples An addition or correction to our examples labels Feb 20, 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.

Looks great? Do you have PRs open to fix the remaining text wrap bugs? Could you link them here?

examples/ui/text_wrap.rs Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Feb 20, 2023

Looks great? Do you have PRs open to fix the remaining text wrap bugs? Could you link them here?

I've got one big thing I'm just cleaning up now, it's a mess internally but the output looks great and, it seems to fix all the remaining text layout and wrapping issues. I'll put in a PR tonight or tomorrow morning. It will need more work and some other people's input though, as it makes quite a lot of changes I'm not sure about.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link

@omwda omwda left a comment

Choose a reason for hiding this comment

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

All looks good, pointed out a couple of typos that were freaking the CI. Should also have an entry in the examples/README.md. No text-wrapping behavior worked on my environment (Metal), but that's an Issue for another day.

Cargo.toml Outdated Show resolved Hide resolved
examples/ui/text_wrap.rs Outdated Show resolved Hide resolved
@ickshonpe
Copy link
Contributor Author

All looks good, pointed out a couple of typos that were freaking the CI. Should also have an entry in the examples/README.md. No text-wrapping behavior worked on my environment (Metal), but that's an Issue for another day.

The no text-wrapping is expected as text-wrapping isn't working correctly in bevy main atm. I made this originally as a test case for finding bugs in the text implementation.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Feb 21, 2023

Looks great? Do you have PRs open to fix the remaining text wrap bugs? Could you link them here?

#7761 #7779

@alice-i-cecile
Copy link
Member

I think you meant to link #7779 :)

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 21, 2023
@doup
Copy link
Contributor

doup commented Mar 7, 2023

I'm not sure about this example:

  • I like it as debugging example as it helps review that the node size is correct after the line breaks are applied; specially interesting when mixed with the Space* cases.
  • But I don't like it for an user, as it mixes two concepts: text wrapping and flex layout.

Otherwise, it looks great. :-)

@ickshonpe
Copy link
Contributor Author

I'm not sure about this example:

  • I like it as debugging example as it helps review that the node size is correct after the line breaks are applied; specially interesting when mixed with the Space* cases.
  • But I don't like it for an user, as it mixes two concepts: text wrapping and flex layout.

Otherwise, it looks great. :-)

Yep it's more of a debugging example, if you look at this in Bevy 0.9 it's a horrible mess. The space cases are especially important to get right.

Maybe there should be a separate examples section for debugging examples full of sanity checks and edge cases like this.

@alice-i-cecile
Copy link
Member

Maybe there should be a separate examples section for debugging examples full of sanity checks and edge cases like this.

Yeah, stress_tests serves a similar role. torture_tests is probably a bit too off-color, but I'd support the creation of a similar folder, either specific to UI or in general.

@doup
Copy link
Contributor

doup commented Mar 7, 2023

Maybe just append _debug to the example name and state it in the example description. Like text_debug does?

image

@ickshonpe
Copy link
Contributor Author

Maybe just append _debug to the example name and state it in the example description. Like text_debug does?

image

Yep, I like that idea.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Mar 8, 2023

Another important use case with this example is to make sure that the text systems respond correctly to window resizing, which is one of the places where the previous attempts to fix text wrapping tended to fail.

@doup
Copy link
Contributor

doup commented Mar 9, 2023

I'm trying to tweak this example to use gap/padding for extra style points… but it breaks when width is not explicitly set. HTML/CSS correctly handles text overflow. Note that width is calculated from flex basis/shrink/grow.

I'm not sure if this is related to #7779; it might be a bug on its own.

Commit with gaps: doup@fc0e008

Current: col width < text width

image

With gap: col width > text width

Works fine 👏 (because text width is smaller than column width)
image

With gap: col width < text width

On window resize, text doesn't overflow. Column width should be smaller.
image

Sanity check with HTML/CSS

HTML example, text-overflow is handled correctly:
https://gist.github.com/doup/78710118d8e54fd89b89d587798f6452

image

cc @nicoburns

@nicoburns
Copy link
Contributor

@doup I think this might be due to overflow: hidden not being handled properly (indeed Taffy currently doesn't allow you to set overflow at all). I think the workaround at the moment would be to set min-width: 0 on the columns. But probably we ought to support overflow?

See w3c/csswg-drafts#1865.

@doup
Copy link
Contributor

doup commented Mar 10, 2023

I don't understand why, but min-width: 0 works. I've created an issue; we can follow there and stop spamming this PR. ^_^U

Btw, I've found another issue, new ticket created: #8017.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

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.

I echo other comments that combining text layout with flex alignment makes this a little confusing. I'd prefer if this was just text layout options. But this generally seems fine.

@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 Apr 23, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 24, 2023
Merged via the queue into bevyengine:main with commit bb37ae4 Apr 24, 2023
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-Examples An addition or correction to our examples 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.

5 participants