android: fix custom top bar title Fill alignment#8325
Open
phuongwd wants to merge 3 commits into
Open
Conversation
…IN_PX wix#8182 added Fill alignment support to `makeTitleAtMostWidthMeasureSpec` by switching the MeasureSpec mode from `AT_MOST` to `EXACTLY` when `isFill` is set. The else branch, however, still subtracts `2 * DEFAULT_LEFT_MARGIN_PX` (32dp) from `containerWidth` for all non-Center modes - including Fill. The 32dp reservation is the Material-style breathing room for left-aligned titles; it is incorrect for Fill, whose intent is to occupy the entire space between the left and right button bars. The visible symptom is a 32dp gap on the right side of any `alignment: 'fill'` custom title component: the React component is measured at `containerWidth - 32dp` but `TitleAndButtonsContainer.onLayout` positions it at `[leftBarWidth, containerWidth - rightBarWidth]`, so the rendered title is 32dp narrower than its layout slot. With no button bars this collapses to a 32dp gap on the right (or the left in RTL). This change splits the else branch so that Fill gets `availableWidth` without the 32dp subtraction, while the left/default alignment branch keeps the existing 16dp x 2 reservation. Related: closes the gap reported in wix#6456 for Fill-aligned title components. Co-Authored-By: Phuong the Cyborg <phuongthecyborg@gmail.com>
Companion to the previous commit (Measurer fix). With the Measurer producing an EXACTLY width of `containerWidth - leftBarWidth - rightBarWidth` for Fill, the title was still positioned via `resolveHorizontalTitleBoundsLimit` which had no knowledge of Fill and applied the same `DEFAULT_LEFT_MARGIN_PX` reservation as left-aligned titles. The result: the React component is measured at the full available width but laid out in a slot 16dp narrower on each side - visible as a margin gap on both edges of the rendered title. This commit: - Adds `isFill: Boolean = false` to `resolveHorizontalTitleBoundsLimit`. When true and not center, returns `[resolvedLeftBarWidth, parentWidth - resolvedRightBarWidth]` with the RTL flip honoured the same way as for left-aligned titles. Existing callers default the new parameter to false; no behaviour change for them. - Threads `isFill` from `TitleAndButtonsContainer.onLayout` into the helper so Fill positioning matches the Measurer-produced size. After both commits, a Fill-aligned custom title component fills the entire horizontal space between left and right button bars (or the entire container width when no buttons are present). Co-Authored-By: Phuong the Cyborg <phuongthecyborg@gmail.com>
phuongwd
changed the title
phuongwd
changed the title
The Fill alignment regression that motivated this PR slipped through review because the existing TitleAndButtonsContainerTest Fill assertion codified the buggy left-aligned-with-margins position, and makeTitleAtMostWidthMeasureSpec itself had zero direct unit tests. This commit: - Adds Robolectric tests for `makeTitleAtMostWidthMeasureSpec` covering Fill (no buttons + with buttons), Center, and left/default. Asserts on both `MeasureSpec.getMode()` and `MeasureSpec.getSize()` to lock the mode-vs-margin matrix that the regression turned on. - Adds Robolectric tests for `resolveHorizontalTitleBoundsLimit(isFill = true)` covering no-buttons, with-buttons LTR, and with-buttons RTL cases. - Corrects the Fill branch of `TitleAndButtonsContainerTest.onLayout - should measure and layout children when alignment changes` to assert the actual Fill intent (`left = 0`, `right = UUT_WIDTH`) instead of the previous `DEFAULT_LEFT_MARGIN_PX` reservation, which was a left-aligned expectation in a test labelled Fill. Co-Authored-By: Phuong the Cyborg <phuongthecyborg@gmail.com>
phuongwd
force-pushed
the
fix/title-fill-margin-subtraction
branch
from
5ea9d51 to
462c6f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes a regression introduced in PR #8182: Fill-aligned custom title components on Android still render with a margin gap on both edges instead of spanning the entire space between the left and right button bars. Reproducible on upstream
master.Three layered fixes plus tests, in three reviewable commits:
1.
makeTitleAtMostWidthMeasureSpec- drop the 32dp reservation for Fill#8182 added the
isFillparameter and switched the MeasureSpec mode fromAT_MOSTtoEXACTLYfor Fill, but the else branch still subtracts2 * DEFAULT_LEFT_MARGIN_PX(32dp). That reservation is the Material-style breathing room for left-aligned titles - wrong for Fill, whose intent is edge-to-edge.2.
resolveHorizontalTitleBoundsLimit+TitleAndButtonsContainer.onLayout- position Fill edge-to-edgeEven with #1, Fill titles were laid out via
resolveHorizontalTitleBoundsLimit, which had no Fill awareness and applied the same 16dp reservation as left-aligned titles. Result: the React component is measured at the full available width but laid out in a slot 16dp narrower on each side. This commit addsisFill: Boolean = falseto the helper (default keeps existing callers unchanged) and threads the flag fromonLayout.3. Tests - Robolectric coverage for the matrix that turned the regression on
makeTitleAtMostWidthMeasureSpechad zero direct unit tests, and the existing Fill assertion inTitleAndButtonsContainerTestcodified the buggy left-aligned-with-margins position. This commit:makeTitleAtMostWidthMeasureSpeccovering Fill / Center / left-aligned with and without button bars, asserting on bothMeasureSpec.getMode()andMeasureSpec.getSize().resolveHorizontalTitleBoundsLimit(isFill = true)covering no-buttons / with-buttons / RTL.TitleAndButtonsContainerTest.onLayout - should measure and layout children when alignment changesto assert the actual Fill intent (left = 0,right = UUT_WIDTH).Related
isFillplumbing without the margin handling and without a Robolectric assertion on the measured width.Validation
Verified end-to-end on a physical Samsung Galaxy S21 (Android 15) by capturing matched before/after pairs against the same playground build. Repro builds: master
1afec76baevs this PR462c6f53ca. A customTopBarTitleTestScreenmounts a 32×32 red marker asleftButton.componentplus a long Fill-aligned title; the red square makes the leftover gap obvious.State 1 - Fill title with subtitle:
1afec76bae)State 2 - Fill title without subtitle (the original report from #6456):
1afec76bae)In the "after" shots the title sits flush against the red marker, the ~32dp leftover gap is gone, and a few more characters of the long title fit before truncation - matching the Fill-aligned edge-to-edge intent.
New Robolectric tests added in this PR exercise the matrix that turned the regression on. Local
yarn test-unit-androidrun pending.What is not yet here / open questions
toHaveRect) on the Fill-aligned title's right edge - happy to add this if you would prefer it land in this PR.RNNReactTitleView.mm(_expectedHeight). My device repro is Android-only; iOS Fill appears correct in adjacent testing but a maintainer eye on it would be welcome.