Skip to content

Fix elements hidden behind keyboard in landscape mode - #102433

Merged
NicolasBonet merged 23 commits into
Expensify:mainfrom
software-mansion-labs:@GCyganek/landscape-mode/elements-hidden-behind-keyboard
Oct 12, 2026
Merged

NicolasBonet merged 23 commits into
Expensify:mainfrom
software-mansion-labs:@GCyganek/landscape-mode/elements-hidden-behind-keyboard

Conversation

@GCyganek

@GCyganek GCyganek commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

In landscape the header plus the open keyboard left no vertical room for the focused input. Five screens now wrap their header/top content in CollapsibleHeaderOnKeyboard and the combination of new CollapsibleHeaderOnKeyboardGroup/GroupMember so it collapses while the keyboard is open, and three selection lists move their footer into the list via useShouldFooterBeInsideList so the confirm button scrolls with the data instead of eating the remaining space.

Fixed Issues

$ #102082
$ #102047
$ #102122
$ #102120
$ #102119
$ #102899
PROPOSAL: N/A

Tests

1. Create expense

  1. Launch app
  2. Tap create expense
  3. Turn to landscape
  4. Try to focus on amount or merchant row
  5. Verify Amount/merchant row is not hidden behind keypad in expense creation page

2. Spend screen

  1. Launch app
  2. Go to spend
  3. Keep cursor on search box
  4. Turn landscape
  5. Note search box hidden behind keypad
  6. Verify Search box is not hidden behind keypad in spend page.

3. Wallet

  1. Launch app
  2. Go to account - wallet
  3. Tap add bank account
  4. Tap get reimbursed/ make payment
  5. Turn landscape
  6. Try to focus on search bar in where's your bank account located
  7. Verify Search input in where's your bank account located is not hidden behind keypad.

4. Workspace remove fixed distance

  1. Launch app with commuterExclusions beta enabled
  2. Go to workspace settings - distance rate - settings
  3. Tap exclude commute - Remove fixed distance
  4. Tap distance input
  5. Turn landscape
  6. Note distance input hidden behind keypad
  7. Verify Remove fixed distance input is not hidden behind keypad.

5. Workspace additional approver

  1. Launch app
  2. Go to workspace settings
  3. Add more than 20 members
  4. Go to workflows - approver - edit
  5. Tap additional approver
  6. Turn landscape and focus on find a member row
  7. Verify that in set approver page, member field is not hidden behind keypad.

6. Workspace company card edit name

Pre-condition: Keep font size to maximum

  1. Launch app
  2. Go to workspace settings
  3. Create a commercial feed ( american express)
  4. Open the feed settings
  5. Turn to landscape and try to focus input
  6. Verify Edit Card feed input is not hidden behind keypad in landscape mode.
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Screen.Recording.2026-09-29.at.14.33.54.mov
Screen.Recording.2026-09-29.at.14.34.22.mov
Screen.Recording.2026-09-29.at.14.34.44.mov
Screen.Recording.2026-09-29.at.14.36.24.mov
Screen.Recording.2026-09-29.at.14.37.26.mov
Screen.Recording.2026-10-08.at.11.37.36.mov
iOS: Native
Screen.Recording.2026-09-29.at.14.52.44.mov
Screen.Recording.2026-09-29.at.15.29.42.mov
Screen.Recording.2026-10-08.at.11.36.16.mov

@GCyganek

Copy link
Copy Markdown
Contributor Author

@Expensify/design here I'm fixing a few places where inputs are hidden behind keyboard in landscape mode. I want to ask which of these solutions are better in your opinion on Spend screen and when creating expense:

  1. Collapsible header only:
Screen.Recording.2026-09-28.at.16.58.14.mov
Screen.Recording.2026-09-28.at.16.57.47.mov
  1. Collapsible header and collapsible tab selector:
Screen.Recording.2026-09-28.at.16.59.19.mov
Screen.Recording.2026-09-28.at.17.02.03.mov

First option should give enough space and is the easiest one, the second one gives more space but the header and tab selector have separate collapsible containers as it's hard to have one in current setup as they are place in different places in the code and it may look weird

@dubielzyk-expensify

Copy link
Copy Markdown
Contributor

2 obviously looks and feels better, but if we know it's not gonna be working reliably like that then option 1 feels okay. I very much prefer two though haha.

@GCyganek

Copy link
Copy Markdown
Contributor Author

Second option it is then 👍

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

@GCyganek

Copy link
Copy Markdown
Contributor Author

@codex please review

@GCyganek

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

The refactor looks correct. Moving the shared logic into useCollapsibleHeader + CollapsibleHeaderView keeps the single-header behavior the same, and I didn't find a blocking logic bug. Before merge, I'd fix the PR description and look at points 2–3.

  1. The PR has no real test or QA steps. Tests and QA only say "no errors in the JS console", and every screenshot section is empty. That covers 5 issues and a new native-only component. Please add per-screen landscape steps: focus the input → header/tabs collapse → input stays visible → close keyboard → header restores. Also add videos. The checklist says unit tests were added, but none cover Group/GroupMember, and Codecov shows coverage dropped.

  2. Commuter exclusions: the in-list Save button picks up offline pending opacity. With the keyboard open in landscape, the footer moves into the list inside OfflineWithFeedback (PolicyCommuterExclusionsPage.tsx:255-272). When a commuterExclusions update is pending offline, needsOpacity (OfflineWithFeedback.tsx:99) dims the button. Outside the list it isn't dimmed. It's minor, but the button now looks different depending on keyboard state.

  3. A group member drops its height on unmount but never re-adds it. The effect in GroupMember/index.native.tsx:30 calls unregisterMember whenever the group value changes. Only onLayout adds a height back, and in landscape it skips unchanged heights. Today this works only because React Compiler keeps the context value stable. If the compiler ever bails on Group, the members' heights silently drop to 0 after any Group re-render, and the header stops collapsing. Two safer options:

    • Depend on group?.unregisterMember only.
    • Re-report the last measured height in the effect body.
Nits
  • Group/index.native.tsx:79 repeats almost all of the keyboard-threshold reaction in index.native.tsx:46, plus the window, focus, and landscape shared values. Consider one shared hook that returns "available height above the keyboard", so the two can't drift apart.
  • SUBHEADER_HEIGHT in InternationalDepositAccountContent.tsx:79 assumes every step has a textHeadlineLineHeightXXL title with mb6. BankAccountDetails uses mb3 for its currency header, and Confirmation has a two-line header. A short comment on why an approximate value is fine would help.
  • The refactor removed two useful comments: "Uses height animation (not translateY) so the freed space is reclaimed…" and the tab-bar example for collapsibleHeaderOffset.

I didn't run the app. These changes are native-only, so the web build renders Group/GroupMember as pass-throughs.

@GCyganek

Copy link
Copy Markdown
Contributor Author

@MelvinBot please review bro

@MelvinBot

Copy link
Copy Markdown
Contributor

Follow-up review (up to 85975e0)

The new commits fix 3 of my 4 earlier points, and I found no new bugs. The only earlier point still open is the commuter exclusions Save button, which is minor.

Fixed:

  • Test steps: the PR description now has landscape steps for all 5 screens.
  • Member heights: GroupMember now re-adds its last height after cleanup (GroupMember/index.native.tsx:155-158), so it no longer depends on React Compiler.
  • Duplicated logic: getKeyboardCollapseState, getAvailableHeightForCollapsibleContent, useCollapsibleScreenState, and useKeyboardCollapseMetrics now hold the shared logic. The single header and the group can no longer drift apart.
  • Offset guess: the hard-coded SUBHEADER_HEIGHT is gone. The Wallet step titles are now group members, so their real heights are measured. The removed comments are back too.

Still open (minor):

  • Commuter exclusions: with the keyboard open in landscape, the Save button still moves inside OfflineWithFeedback (PolicyCommuterExclusionsPage.tsx:255-272). If an update is pending offline, the button is dimmed there but not elsewhere. If that's acceptable, a short reply here is enough.
Nits
  • The Wallet test step only covers the country selection step. The new group members on the bank information, account holder, account type, and account details steps have no test step. One extra step would cover them, for example: focus a field on the bank information step → the header and step title collapse.
  • The Confirmation step has no text input, so wrapping its title in a GroupMember (Confirmation.tsx) does nothing. It's harmless, so keep it if you want every step to look the same.
  • CountrySelectionList is also used in BankAccountPurposePage outside any group. There the GroupMember never collapses, which matches the old behavior. Just confirming that's intended.

I didn't run the app. These changes only affect native, and the web build renders Group/GroupMember as pass-throughs.

@GCyganek
GCyganek marked this pull request as ready for review September 29, 2026 13:30
@GCyganek
GCyganek requested review from a team as code owners September 29, 2026 13:30
@melvin-bot
melvin-bot Bot requested review from trjExpensify and truph01 and removed request for a team September 29, 2026 13:30
@melvin-bot

melvin-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

@truph01 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

shouldUpdateFocusedIndex
alternateNumberOfSupportedLines={2}
disableKeyboardShortcuts
listFooterContent={shouldFooterBeInsideList ? footer : undefined}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use footerContent + shouldFooterBeInsideList here too?

The other lists in this PR pass footerContent with shouldFooterBeInsideList. Would it make sense to do the same here and drop the listFooterContent / !shouldFooterBeInsideList && footer toggle?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using footerContent + shouldFooterBeInsideList would change the layout in portrait mode -> SelectionList is wrapped in OfflineWithFeedback, so the errors would be displayed below the button instead of between the list and the button

<View style={styles.ph5}>
<Text style={[styles.textHeadlineLineHeightXXL, styles.mb6]}>{translate('addPersonalBankAccount.countrySelectionStepHeader')}</Text>
</View>
<CollapsibleHeaderOnKeyboardGroupMember>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BankAccountPurposePage and CollectDepositAccount also render this list without a CollapsibleHeaderOnKeyboardGroup, so the header never collapses there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added, thanks!

@truph01

truph01 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T11:12:53.114433Z e3adf74 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: e3adf74f68

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@GCyganek

Copy link
Copy Markdown
Contributor Author

I see github actions are currently failing due to Setup node step failure caused by something not added on this PR

@truph01

truph01 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

@GCyganek Could you merge main? The failing checks should be fixed now

@melvin-bot

melvin-bot Bot commented Oct 12, 2026

Copy link
Copy Markdown

We did not find an internal engineer to review this PR, trying to assign a random engineer to #102082 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

@melvin-bot
melvin-bot Bot requested a review from NicolasBonet October 12, 2026 12:27
@NicolasBonet
NicolasBonet merged commit 06ca186 into Expensify:main Oct 12, 2026
35 of 37 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.5.8-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

⚠️ A setup step failed before I could start. This is usually a temporary issue — try again in a few minutes, and if it persists post in #melvinbot-ai.

Run details

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.8-5 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

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

Successfully merging this pull request may close these issues.

7 participants