Open pecoram opened 5 months ago
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.
Just to clarify - is
contain: "width"
equivalent of csstext-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
I was able to achieve the same result as in lightning3:
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.
In this case should this issue be treated as invalid or maybe we want to implement the wrap-word functionality in it's scope?
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?
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.
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
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?
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?
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.
Maybe let's start with word-wrap: [space|wrap] and then decide if we want to implement pretty ?
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
Definitely. Also it is going to be a good reference.
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?
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.
@elsassph so It's ok to keep current implementation and just change word-wrap to [normal | break]?
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.
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.
@wouterlucas I got a bit confused. Do we need both wordWrap
and wordBreak
exposed? Should they be boolean or string literals?
@wouterlucas I got a bit confused. Do we need both
wordWrap
andwordBreak
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?
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.
Yes L2 basic text texture renderer doesn't do word-break, it was only in the "advanced" renderer.
With
contain: "width"
a long text is not wrapped correctly:this issue is for both canvas and sdf