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

Wrong wrapping with contain width and text too long #268

Open
pecoram opened this issue May 3, 2024 · 23 comments
Open

Wrong wrapping with contain width and text too long #268

pecoram opened this issue May 3, 2024 · 23 comments

Comments

@pecoram
Copy link
Contributor

pecoram commented May 3, 2024

With contain: "width" a long text is not wrapped correctly:

image

this issue is for both canvas and sdf

@marcel-danilewicz-consult-red
Copy link
Contributor

Just to clarify - is contain: "width" equivalent of css text-wrap: wrap? If yes, then I think, that this behavior may be correct. I checked https://developer.mozilla.org/en-US/docs/Web/CSS/text-wrap and it behaves similarly. The text overflows only if it is continuous, without whitespaces.

@pecoram
Copy link
Contributor Author

pecoram commented Jul 5, 2024

Just to clarify - is contain: "width" equivalent of css text-wrap: wrap? If yes, then I think, that this behavior may be correct. I checked https://developer.mozilla.org/en-US/docs/Web/CSS/text-wrap and it behaves similarly. The text overflows only if it is continuous, without whitespaces.

contain: "width" must behave as text-wrap: wrap so if a word is too long the excess part must wrap. Just like the example on the page you linked

image

@marcel-danilewicz-consult-red
Copy link
Contributor

I was able to achieve the same result as in lightning3:
image

@philippe-wm
Copy link

Yes it's expected. L2 has a wrap-word option in the advanced text renderer which will break a single too-long word, but it has a cost.

@marcel-danilewicz-consult-red
Copy link
Contributor

marcel-danilewicz-consult-red commented Jul 5, 2024

In this case should this issue be treated as invalid or maybe we want to implement the wrap-word functionality in it's scope?

@marcel-danilewicz-consult-red
Copy link
Contributor

I tried tinkering with the code in case we wanted to implement this. Is this the expected behavior for "wrap-word" or should the overflow at the bottom not happen?
image

@frank-weindel
Copy link
Collaborator

We wouldn't want this to be a new contain setting. "Wrap Word" would be relevant to both the "width" and "both" contain modes. So it should be its own setting.

So whatever the behavior of those two contain modes will need to remain. "width" allows for unlimited lines. "both" truncates the text so it doesn't cross the specified height on the node.

@pecoram
Copy link
Contributor Author

pecoram commented Jul 10, 2024

In my opinion it is wrong to wrap with spaces but if we want we can add a separate parameter that we call wordWrap and if it is true it truncates the word and not the last space

@marcel-danilewicz-consult-red
Copy link
Contributor

Hi @wouterlucas
I'd like to clarify the requirements for this functionality. I see two solutions:
a) any word, that exceeds the width limit should wrap
b) the word, that exceeds the width limit should be moved to the next line. If it stil exceeds the limit it should wrap until it fits
Is either one correct? Are there any other requirements?

@wouterlucas
Copy link
Contributor

We should provide options instead of a boolean, because implementing an algorithm that tries to catch all scenarios gracefully will very likely perform very poorly as @elsassph pointed out.

We could have the dev point out what he wants explicitly.

My suggestion:

word-wrap: [space|wrap|pretty]
space - Breaks on spaces, as efficient as possible
wrap - Does a normal wrap, breaks on spaces if available if it still doesn't fit break on character
pretty - Like L2, breaks on space if possible, breaks on character but tries to fill/remove characters if needed to reduce orphans

Flex and wrap algo's can be detrimental to performance and hopefully space will work for the majority of the cases in latin languages without sacrificing too much. We could also start with space/wrap as a first phase?

@elsassph
Copy link
Contributor

elsassph commented Jul 19, 2024

L2 had just an extra wordBreak flag in addition to regular wrapping options, and it was only breaking a single too-long word as in the linked issue. It seems sensible enough to me - only the implementation in L2 wasn't great.

The suggested wrap doesn't make sense to me - I don't imagine one would want systematically breaking words. This is scope/complexity creep IMHO.

@marcel-danilewicz-consult-red
Copy link
Contributor

marcel-danilewicz-consult-red commented Jul 23, 2024

Maybe let's start with word-wrap: [space|wrap] and then decide if we want to implement pretty ?

@elsassph
Copy link
Contributor

Would be preferable to align with CSS rather than invent new names like "pretty" https://developer.mozilla.org/en-US/docs/Web/CSS/word-break

@marcel-danilewicz-consult-red
Copy link
Contributor

Definitely. Also it is going to be a good reference.

@wouterlucas
Copy link
Contributor

Would be preferable to align with CSS rather than invent new names like "pretty" https://developer.mozilla.org/en-US/docs/Web/CSS/word-break

https://developer.mozilla.org/en-US/docs/Web/CSS/text-wrap#pretty 🙃 - however I agree that's more of a flex / text layout property over just word breaks.

And the line between text layouting and word breaking is blurry, if we want to limit this to word break functionality only. I agree that word-break is a fine guidance. Likely limited to
normal, break all or keep all?

@elsassph
Copy link
Contributor

Weeeell the CSS pretty option has a fair explanation :D

For now I think we can support normal and break - and unlike what I suggested it seems that the breaking approach that @marcel-danilewicz-consult-red started would be the right thing to do.

However keep-all option requires testing the unicode range of the characters, so it's a bit too much scope for a first shot.

@marcel-danilewicz-consult-red
Copy link
Contributor

@elsassph so It's ok to keep current implementation and just change word-wrap to [normal | break]?

@elsassph
Copy link
Contributor

I don't get to decide, I just a contributor 😅
I'd be reluctant to make an API change now that it's in v1, so it seems better to add an extra wordBreak option - but that would be following your current implementation.

@wouterlucas
Copy link
Contributor

Agree, exposing wordBreak as TrProp is fine. Both wordWrap and wordBreak are already available internally (as booleans tho) and wrap is only flipped when contain is enabled.

@marcel-danilewicz-consult-red
Copy link
Contributor

@wouterlucas I got a bit confused. Do we need both wordWrap and wordBreak exposed? Should they be boolean or string literals?

@wouterlucas
Copy link
Contributor

@wouterlucas I got a bit confused. Do we need both wordWrap and wordBreak exposed? Should they be boolean or string literals?

I was referring to these, they are available internally for Canvas:
https://github.com/lightning-js/renderer/blob/main/src/core/text-rendering/renderers/LightningTextTextureRenderer.ts#L72-L74

I'd expose them both, wordWrap you already have in #345 - just needs to be hooked up to the Canvas portion and then add wordBreak as TrProp and exposed to both the Canvas and the SDF font renderer. Can also be separate PRs if you like.

And Boolean's are fine I guess?

@wouterlucas
Copy link
Contributor

Oh wait, the wordBreak isn't doing anything in https://github.com/lightning-js/renderer/blob/main/src/core/text-rendering/renderers/LightningTextTextureRenderer.ts#L74 as its always set to false and nothing seems to look at it down below. So that would need to hooked up/changed.

Also having two boolean's would be confusing as what do we do if they're both true. In that case a string with
normal or break is safer.

Sorry for the confusion, the text rendering code is relatively new to me and the wordBreak option on the Canvas side threw me off.

@elsassph
Copy link
Contributor

Yes L2 basic text texture renderer doesn't do word-break, it was only in the "advanced" renderer.

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

No branches or pull requests

6 participants