fix: default UiText textWrap to wrap instead of nowrap - #9712
Conversation
When a scene omitted 'textWrap', SetupLabel fell back to WhiteSpace.NoWrap, contradicting the proto spec where TW_WRAP is the zero-value default. The generated PBUiText.TextWrap accessor already returns TW_WRAP when the field is absent from the wire, so the HasTextWrap branch was redundant and its else clause actively overrode that default. Reading TextWrap directly removes the redundant check and derives the default from the proto instead of hardcoding it a second time. This restores the behaviour that predated #1387, which introduced the whiteSpace block: before it, labels kept the WhiteSpace.Normal set by SetElementDefaultStyle, which that method still applies at instantiation. Only scenes using the raw SDK are affected -- react-ecs always sends the field explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚦 CI StatusNew build in progress, come back later! Warnings not reduced: 13116 => 13116 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (15)All Unity tests passed ✅
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9712
STEP 2 — Root-cause check: PASS
The PR fixes the actual root cause. The old code:
if (model.HasTextWrap)
labelToSetup.style.whiteSpace = model.TextWrap == TextWrap.TwWrap ? WhiteSpace.Normal : WhiteSpace.NoWrap;
else
labelToSetup.style.whiteSpace = WhiteSpace.NoWrap;The else branch hardcoded WhiteSpace.NoWrap as the fallback when TextWrap was absent from the wire. However, the proto spec defines TW_WRAP (value 0) as the zero-value default — confirmed in the generated UiText.gen.cs:
private readonly static TextWrap TextWrapDefaultValue = TextWrap.TwWrap;
public TextWrap TextWrap {
get { if ((_hasBits0 & 8) != 0) { return textWrap_; } else { return TextWrapDefaultValue; } }
}The accessor already returns TwWrap when the field is absent, so reading model.TextWrap directly is correct — the redundant HasTextWrap check is removed and the proto-defined default flows through naturally.
STEP 3 — Design & integration: PASS
No new units, types, or long-lived state introduced. The fix is correctly located in SetupLabel (UiElementUtils.cs), which is the utility method responsible for mapping proto model fields to UI Toolkit style properties. No lifecycle, subscription, or ownership changes.
I also checked TMPProSdkExtensions.cs — it uses HasTextWrapping on a different component (PBTextShape, a boolean field for the legacy TextShape component), not the TextWrap enum on PBUiText. No similar bug pattern there.
STEP 4 — Member audit: PASS
No new public members added or changed.
STEP 5 — Line-level review: No issues found
Code change: Clean one-liner that correctly simplifies the conditional by trusting the proto-generated default. Both the absent case (TwWrap → WhiteSpace.Normal) and the explicit TwNoWrap case (WhiteSpace.NoWrap) are handled correctly.
Tests: Two well-structured tests following AAA pattern with NUnit:
WrapTextWhenTextWrapIsAbsent— verifies the default (absent field →WhiteSpace.Normal) ✅NotWrapTextWhenTextWrapIsExplicitlyNoWrap— verifies explicitTwNoWrap→WhiteSpace.NoWrap✅
Both tests cover the behavioral boundary that the bug affected. No resource leaks, no allocation issues, no security concerns.
STEP 6 — Complexity: SIMPLE
Two files, ~36 lines of meaningful changes (mostly test additions), straightforward bug fix in a utility method.
STEP 7 — QA: YES
Changes runtime UI text rendering behavior — scenes that previously had unwrapped text by default will now correctly wrap.
STEP 8 — Non-blocking warnings: None
Security review: No security issues found
Pure UI rendering logic change with no user input handling, authentication, secrets, or network changes.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Bug fix in a static utility method (UiElementUtils.SetupLabel) with corresponding unit tests — no ECS system, async, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
This comment has been minimized.
This comment has been minimized.
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [✔️ ] Chat and multiplayer
- [ ✔️] Profile card
Evidence:
|
PR #9712, run #32129566024 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
Fixes #9671
When a scene omitted 'textWrap', SetupLabel fell back to WhiteSpace.NoWrap, contradicting the proto spec where TW_WRAP is the zero-value default. The generated PBUiText.TextWrap accessor already returns TW_WRAP when the field is absent from the wire, so the HasTextWrap branch was redundant and its else clause actively overrode that default.
Reading TextWrap directly removes the redundant check and derives the default from the proto instead of hardcoding it a second time.
QA TEST STEPS
Use the build from this PR to enter the ZONE/SEPOLIA world
sdk7testscenes.dcl.ethat position80,-3and confirm at the top-right UI panels for text wrap that you see: