Fix inline-edit pickers overlapping the cell being edited on small-height screens - #100508
Fix inline-edit pickers overlapping the cell being edited on small-height screens#100508MelvinBot wants to merge 1 commit into
Conversation
When the picker could not fit below the cell being inline-edited, usePopoverEditState returned the cell's top coordinate while the pickers kept TOP anchor alignment, so the picker's top edge was placed over the cell rather than flipping above it. Pick the side with room, pin the popover's bottom edge to the top of the cell when opening above, and shrink the category/tag pickers to the space actually available (down to a minimum usable height) so they never cover the cell. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
To take ownership of the branch so you can push updates: Then you can close this PR and open a new one from your fork. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🤖 Android test steps verification results: On standalone Android NewDot, Reports > All Expenses renders the narrow card list (
The emulator is 1080x2400 @420dpi (~411dp wide), but native short-circuits the breakpoint regardless of device size, so no Android configuration reaches the wide table. The positioning fix needs to be validated on web at a large window width. |
|
🤖 Web test steps verification results: ❌ fail I could not run any of the documented steps on dev NewDot web. Two blockers stopped the run before a single picker opened: the CI test session was minted as a brand-new account, so Spend > Expenses is empty (no transaction table, no Category/Tag/Date columns to inline-edit); and inline editing is unreachable in this headless browser regardless of data, because hover is inert. hasHoverSupport/index.ts:6 evaluates
Combined with the Android run on this PR, which found inline editing is wide-layout-only and therefore unreachable on native, PR #100508 has no runtime verification on either platform yet. Making the web path work needs two harness changes: seed the session account with a workspace plus several categorised and tagged expenses before handing over the session, and launch the browser so it advertises view run · no recording available |
|
@MelvinBot This PR name looks not correct? isn't we still open the picker above or bottom the cell not next to it? |
MelvinBot
changed the title
|
Good catch — "next to" reads as horizontally beside the cell, and that's not what the code does. The picker only ever moves vertically: below the cell when there's room, fully above it when there isn't, shrinking to fit when neither side can take the full height. The horizontal position is unchanged — it stays pinned to the cell's left (or right) edge via I renamed the PR to:
and fixed the same phrase in the One leftover: the commit subject on the branch still carries the old "next to the cell" wording. I left it alone rather than amending an already-pushed commit — let me know if you'd rather it be reworded and I can add a follow-up commit with a corrected message. |
|
@MelvinBot rename to "Fix inline-edit pickers overlapping the cell being edited on small-height screens". Also update description if needed. |
MelvinBot
changed the title
|
Renamed the PR to "Fix inline-edit pickers overlapping the cell being edited on small-height screens". I left the description as-is — it already matches the new title. The Explanation of Change opens with the picker "opened on top of the cell being edited instead of above or below it. It reproduced whenever the window was short, or when the row sat low enough that the full-height picker did not fit below it", which is exactly what the new title says. Nothing in the body contradicts the rename. One thing worth folding into the description if you want it there — I verified it on an Android emulator on the last run, but did not add it myself since it is outside the rename:
Say the word and I'll add it to the body. |
|
Two follow-up fixes for the inline-edit date picker (the category/tag pickers are fine): 1. Calendar top gets clipped on short windowsWhen the picker opens above the cell ( 
Root cause: 2. Date picker sits flush against the window edge; category leaves a small gapOn a short window the clamped calendar hugs the window's top/bottom edge, whereas the category picker keeps an 8px gap (it reserves 
Fix code diffdiff --git a/src/components/PopoverWithMeasuredContent/types.ts b/src/components/PopoverWithMeasuredContent/types.ts
--- a/src/components/PopoverWithMeasuredContent/types.ts
+++ b/src/components/PopoverWithMeasuredContent/types.ts
@@ -20,6 +20,13 @@ type PopoverWithMeasuredContentProps = Omit<PopoverProps, 'anchorPosition'> & {
/** Whether to skip re-measurement when becoming visible (for components with static dimensions) */
shouldSkipRemeasurement?: boolean;
+
+ /**
+ * Breathing room (in px) to keep between the popover and the window edges when it must be clamped inside the
+ * window. Content that can't shrink (e.g. the calendar) would otherwise sit flush against the edge; a positive
+ * value keeps the same gap the shrinking pickers leave. Defaults to 0 (flush), preserving existing behavior.
+ */
+ windowMargin?: number;
};
export default PopoverWithMeasuredContentProps;
diff --git a/src/components/PopoverWithMeasuredContent/PopoverWithMeasuredContentBase.tsx b/src/components/PopoverWithMeasuredContent/PopoverWithMeasuredContentBase.tsx
--- a/src/components/PopoverWithMeasuredContent/PopoverWithMeasuredContentBase.tsx
+++ b/src/components/PopoverWithMeasuredContent/PopoverWithMeasuredContentBase.tsx
@@ -55,6 +55,7 @@ function PopoverWithMeasuredContentBase({
shouldEnableNewFocusManagement,
shouldMeasureAnchorPositionFromTop = false,
shouldSkipRemeasurement = false,
+ windowMargin = 0,
...props
}: PopoverWithMeasuredContentProps) {
@@ -184,11 +185,21 @@ function PopoverWithMeasuredContentBase({
if (anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP) {
const top = adjustedAnchorPosition.top + positionCalculations.verticalShift;
const maxTop = windowHeight - popoverHeight - positionCalculations.verticalShift;
- result.top = Math.min(Math.max(positionCalculations.verticalShift, top), maxTop);
+ let clampedTop = Math.min(Math.max(positionCalculations.verticalShift, top), maxTop);
+ if (windowMargin > 0) {
+ // Keep `windowMargin` of breathing room from the window edges instead of sitting flush against them.
+ clampedTop = Math.min(Math.max(clampedTop, windowMargin), Math.max(windowHeight - popoverHeight - windowMargin, windowMargin));
+ }
+ result.top = clampedTop;
}
if (anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM) {
- result.bottom = windowHeight - (adjustedAnchorPosition.top + popoverHeight) - positionCalculations.verticalShift;
+ const bottom = windowHeight - (adjustedAnchorPosition.top + popoverHeight) - positionCalculations.verticalShift;
+ // Keep the popover inside the window. When the space above the anchor is smaller than the popover, pinning
+ // the bottom edge would push the top edge above the window and clip the content, so cap `bottom` at the
+ // value that keeps the top edge inside the window, leaving `windowMargin` of breathing room from the edge.
+ const maxBottom = windowHeight - popoverHeight - windowMargin;
+ result.bottom = windowMargin > 0 ? Math.min(Math.max(bottom, windowMargin), Math.max(maxBottom, windowMargin)) : Math.min(bottom, maxBottom);
}
return result;
@@ -201,6 +212,7 @@ function PopoverWithMeasuredContentBase({
windowHeight,
popoverHeight,
shouldMeasureAnchorPositionFromTop,
+ windowMargin,
]);
diff --git a/src/components/DatePicker/types.ts b/src/components/DatePicker/types.ts
--- a/src/components/DatePicker/types.ts
+++ b/src/components/DatePicker/types.ts
@@ -125,6 +125,14 @@ type DatePickerProps = {
* Used by inline editing flows that require background dimming.
*/
shouldEnableMonthYearBackdropInNarrowPane?: boolean;
+
+ /**
+ * Measure the calendar's real rendered height for positioning instead of assuming the static
+ * `POPOVER_DATE_MIN_HEIGHT`. The calendar's height varies by month (5 vs 6 week rows) and exceeds that constant, so
+ * when the popover is anchored by its bottom edge (opening above the anchor) an assumed-too-short height lets the
+ * real top edge overflow above the window and get clipped. Enable this where the popover can open above its anchor.
+ */
+ shouldMeasureContentHeight?: boolean;
} & Omit<BaseTextInputProps & PopoverWithMeasuredContentProps, 'anchorRef' | 'children'>;
diff --git a/src/components/DatePicker/DatePickerModal.tsx b/src/components/DatePicker/DatePickerModal.tsx
--- a/src/components/DatePicker/DatePickerModal.tsx
+++ b/src/components/DatePicker/DatePickerModal.tsx
@@ -27,6 +27,12 @@ const popoverDimensions = {
width: CONST.POPOVER_DATE_WIDTH,
};
+// A zero height opts the popover into measuring the calendar's real rendered height (see `shouldMeasureContentHeight`).
+const measuredPopoverDimensions = {
+ height: 0,
+ width: CONST.POPOVER_DATE_WIDTH,
+};
+
function DatePickerModal({
value,
defaultValue,
@@ -46,6 +52,8 @@ function DatePickerModal({
shouldPositionFromTop = false,
forwardedFSClass,
shouldEnableMonthYearBackdropInNarrowPane = false,
+ shouldMeasureContentHeight = false,
+ windowMargin,
}: DatePickerProps) {
@@ -83,7 +91,8 @@ function DatePickerModal({
isVisible={isVisible}
onClose={onClose}
anchorPosition={anchorPosition}
- popoverDimensions={popoverDimensions}
+ popoverDimensions={shouldMeasureContentHeight ? measuredPopoverDimensions : popoverDimensions}
+ windowMargin={windowMargin}
shouldCloseWhenBrowserNavigationChanged={shouldCloseWhenBrowserNavigationChanged}
innerContainerStyle={isSmallScreenWidth ? styles.w100 : {width: CONST.POPOVER_DATE_WIDTH}}
anchorAlignment={anchorAlignment}
diff --git a/src/components/Search/SearchList/ListItem/DateCell.tsx b/src/components/Search/SearchList/ListItem/DateCell.tsx
--- a/src/components/Search/SearchList/ListItem/DateCell.tsx
+++ b/src/components/Search/SearchList/ListItem/DateCell.tsx
@@ -77,6 +77,12 @@ function DateCell({date, showTooltip, isLargeScreenWidth, suffixText, shouldUseL
onSelected={handleSave}
anchorPosition={popoverPosition}
anchorAlignment={anchorAlignment}
+ // The calendar's height varies by month and exceeds POPOVER_DATE_MIN_HEIGHT, so measure it: when
+ // opening above the cell (bottom-edge anchored) a too-short assumed height would clip the top.
+ shouldMeasureContentHeight
+ // The calendar can't shrink, so when it's clamped inside a short window keep the same gap from the
+ // window edge that the shrinking category/tag pickers leave, instead of sitting flush against it.
+ windowMargin={CONST.MODAL.POPOVER_MENU_PADDING}
shouldPositionFromTop={!shouldOpenAbove}
minDate={CONST.CALENDAR_PICKER.MIN_DATE}
maxDate={CONST.CALENDAR_PICKER.MAX_DATE} |
|
@MelvinBot Please implement this fix from claude #100508 (comment) |
|
|
|
@MelvinBot Please implement this fix from claude #100508 (comment) |
|
|
|
@MelvinBot Please implement this fix from claude #100508 (comment) |
|
|
2 participants
Explanation of Change
When you inline-edit a transaction row in the table (Reports / All Expenses), the Category picker — and the Tag and Date pickers, which share the same logic — opened on top of the cell being edited instead of above or below it. It reproduced whenever the window was short, or when the row sat low enough that the full-height picker did not fit below it.
Root cause.
usePopoverEditStatemeasured the anchor cell and, when the picker would not fit below it, setisInvertedand returned the cell's top coordinate as the anchor position. But the pickers keptanchorAlignment.vertical: ANCHOR_ORIGIN_VERTICAL.TOP, and withTOPalignmentPopoverWithMeasuredContentBase.tsx:147-150places the popover's top edge atanchorPosition.vertical. So the "inverted" branch never flipped the picker above the cell — it just started the picker at the cell's top edge, i.e. directly over the cell, andcomputeVerticalShiftthen nudged it up only as far as needed to fit in the viewport.shouldMeasureAnchorPositionFromTop={!isInverted}had no effect, because theTOPbranch assignsresult.topregardless of that flag.Secondarily, the picker had a fixed height of
POPOVER_DROPDOWN_MAX_HEIGHT(416), so on a short window it could not fit on either side of the cell and was forced to overlap something.What changed. Following the guidance on the issue ("I wouldn't add that complexity and just shrink the popover"):
usePopoverEditState.tsnow computes the space usable on each side of the cell (reserving the gap to the cell plus the same gap to the window edge), opens below when the preferred height fits there, otherwise takes whichever side has more room, and shrinks the picker to the space actually available — floored at a minimum usable height, below which it stops shrinking and lets the popover be clamped inside the window.anchorAlignmentthat usesANCHOR_ORIGIN_VERTICAL.BOTTOMwhen opening above, so the popover's bottom edge is pinned to the top of the cell. This is the actual overlap fix: being height-independent, it stays correct even for content that can't shrink.CategoryPickerModalandTagPickerModalaccept the resolved height (theirpopoverDimensionswere hard-coded module constants), passed fromCategoryCell/TagCelltogether with the hook'sanchorAlignment.DateCellpassespopoverHeight: CONST.POPOVER_DATE_MAX_HEIGHTto the hook so the side is chosen using the calendar's real height rather than the generic dropdown height. The calendar itself is not shrunk — the bottom-edge anchoring is what keeps it off the cell.PopoverWithMeasuredContentBasemirrors a changed staticpopoverDimensions.heightinto state. Static dimensions skip theonLayoutmeasurement, sopopoverHeightwould otherwise keep its first-render value and the positioning math would still treat a shrunk picker as 416px tall.Result: the picker opens below the cell when there is room, flips fully above it when there is not, and shrinks to fit when neither side can take the full height — so it never covers the cell being edited.
AI Tests
Run locally on this branch:
npm test -- tests/unit/inlineEditing/editableCellHooks.test.tsnpm testover 11 related suites (TableTest,PopoverMenuV2Test,TransactionInlineEdit,TransactionItemRowRBRTest,WorkspaceCompanyCardsTableTest,WorkspaceViewTagsTableTest,CategoryPickerTest,CategoryTagPickerAutoFocusTest,usePopoverPositionTest,DateCellTest,editableCellHooks)npm run lint-changednpm run typechecknpm run spell-changednpm run react-compiler-compliance-check check <changed files>npm run prettieris not defined in this repo'spackage.jsonand App has no Prettier config; formatting is covered bylint-changed.Not verified in a browser. The automated web session available to this run reports no hover capability (
window.matchMedia('(hover: hover) and (pointer: fine)')is false), soHoverablestays disabled and the hover-gated edit pencil inEditableCellnever appears — clicks fall through to row navigation andstartEditingis never reached. The placement logic is covered by the unit tests below instead; the manualTests/QAsteps still need a human on a real browser.New unit tests in
tests/unit/inlineEditing/editableCellHooks.test.ts, stubbing the anchor measurement and window height:anchorAlignment.vertical === TOP,vertical === y + height + padding.shouldOpenAbove === true,anchorAlignment.vertical === BOTTOM,vertical === y - padding(bottom edge above the cell), full height retained.anchorEdge: RIGHTanchors to the right edge of the cell.Fixed Issues
$ #88681
$ #82534
PROPOSAL: #82534 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review". Suggested starting point:
// 1. Open Reports > All Expenses with several expenses in the table.
// 2. Inline-edit the Category cell on a row near the top of the table. Verify the picker opens below the cell and does not cover it.
// 3. Inline-edit the Category cell on a row low in the table. Verify the picker opens fully above the cell and does not cover it.
// 4. Shrink the window to a short viewport (e.g. ~500px tall) and inline-edit a Category cell in the middle of the table. Verify the picker shrinks to fit and still does not cover the cell.
// 5. Repeat 2-4 for the Tag cell and the Date cell.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA steps before marking this PR as "ready for review", or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari