Repository navigation
Avoid nested ScrollViews in landscape bottom-docked confirm modals - #102335
Conversation
Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
The failing checks are unrelated to this PR, so I didn't push a fix. Analysis: This PR changes only
Evidence
Next steps: Once someone fixes |
…LandscapeConfirmModal Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
I merged The failures weren't caused by this PR. They came from a merge race on The merge had no conflicts and doesn't change this PR's own files. I couldn't run Jest or typecheck in my environment, so the new CI run is the verification. |
|
🤖 Android test steps verification results: ❌ fail I couldn't verify the fix on Android. The PR's Tests section is still a TODO, so I used the steps from Landscape - Long scrollable confirmation modals can make content or actions unreachable. The tester signed in and created a workspace with 7 draft reports. The app then froze and stopped responding to touch before it could open the bulk Submit modal. The app was still frozen on a second check about 8 minutes later. The tester can't restart the app, so it ran none of the modal checks (portrait or landscape).
To get a real result, rerun this on a fresh emulator session. It would also help to add test steps to the PR's Tests section. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
Please merge main. |
|
I didn't merge GitHub currently shows the branch as mergeable with no conflicts. I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me. |
|
Please assign me. I reviewed your Proposal from #101333 (comment) as the C+ |
|
Done. I assigned you to this PR and requested your review, since you're the C+ on Landscape - Long scrollable confirmation modals can make content or actions unreachable and you reviewed the proposal. The PR's Tests and QA sections are still placeholders, so a human co-author needs to fill them in before review. |
|
Please merge main |
…LandscapeConfirmModal Co-authored-by: TaduJR <TaduJR@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
🤖 I replaced the PR description with the one you provided. It includes the new Explanation of Change, the 15-report landscape Submit test steps, and your checklist. I left out the closing "Please replace the description with the above one." line. |
|
@MelvinBot The code looks good. It's the same change I tested on iOS in landscape. Two things on the tests:
tests/ui/BaseModalTest.tsximport type ModalContextType from '@components/Modal/ModalContext';
it.each([
[CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED, true, undefined, true],
[CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED, true, false, false],
[CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED, false, undefined, false],
[CONST.MODAL.MODAL_TYPE.CENTERED, true, undefined, false],
])(
'tells children whether a %s modal in landscape=%s with wrap override %s already scrolls them',
(type, isInLandscapeMode, shouldWrapModalChildrenInScrollViewIfBottomDockedInLandscapeMode, expectedIsContentWrappedInScrollView) => {
// Given the orientation is forced and the modal renders its children, so a child can read the context BaseModal provides
jest.doMock('react', () => React);
jest.doMock('@libs/isInLandscapeMode', () => ({
__esModule: true,
default: () => isInLandscapeMode,
}));
jest.doMock('@components/Modal/ReanimatedModal', () => ({
__esModule: true,
default: ({children}: ReanimatedModalProps) => children,
}));
const {default: BaseModal} = jest.requireActual<{default: typeof BaseModalComponent}>('@components/Modal/BaseModal');
const {default: ModalContext} = jest.requireActual<{default: typeof ModalContextType}>('@components/Modal/ModalContext');
let isContentWrappedInScrollView: boolean | undefined;
function ContextReader() {
isContentWrappedInScrollView = React.useContext(ModalContext).isContentWrappedInScrollView;
return null;
}
// When the modal is shown
render(
<BaseModal
isVisible
type={type}
shouldWrapModalChildrenInScrollViewIfBottomDockedInLandscapeMode={shouldWrapModalChildrenInScrollViewIfBottomDockedInLandscapeMode}
>
<ContextReader />
</BaseModal>,
);
// Then children learn they are already inside a vertical scroller only when BaseModal actually adds its landscape ScrollView,
// so they don't nest a second one that would fight it for the drag gesture
expect(isContentWrappedInScrollView).toBe(expectedIsContentWrappedInScrollView);
},
);tests/unit/ConfirmContentTest.tsximport {render, screen, within} from '@testing-library/react-native';
import ModalContext from '@components/Modal/ModalContext';
jest.mock('@components/ScrollView', () => {
const ReactLib = jest.requireActual<typeof React>('react');
return {
__esModule: true,
default: ({children}: {children?: React.ReactNode}) => ReactLib.createElement('mock-scroll-view', {testID: 'prompt-scroll-view'}, children),
};
});
describe('scrollable prompt (shouldEnablePromptScroll=true)', () => {
const prompt = '• Report 1\n• Report 2';
it('puts the prompt in its own ScrollView when the modal does not already scroll its children', () => {
// Given a modal that renders its children without a ScrollView, as a bottom-docked modal does in portrait
// When a long prompt asks to be scrollable
render(
<ModalContext.Provider value={{isContentWrappedInScrollView: false, default: false}}>
<ConfirmContent
title="Test"
onConfirm={jest.fn()}
isVisible
prompt={prompt}
shouldEnablePromptScroll
/>
</ModalContext.Provider>,
);
// Then the prompt scrolls on its own, so the title and buttons stay pinned around the long list
expect(within(screen.getByTestId('prompt-scroll-view')).getByText(prompt)).toBeOnTheScreen();
});
it('renders the prompt without its own ScrollView when the modal already scrolls its children', () => {
// Given a modal that already wraps its children in a ScrollView, as a bottom-docked modal does in landscape
// When a long prompt asks to be scrollable
render(
<ModalContext.Provider value={{isContentWrappedInScrollView: true, default: false}}>
<ConfirmContent
title="Test"
onConfirm={jest.fn()}
isVisible
prompt={prompt}
shouldEnablePromptScroll
/>
</ModalContext.Provider>,
);
// Then the prompt is not nested in a second vertical scroller: iOS would keep the drag in the inner one and Android would
// give it to the outer one, leaving either the buttons or the end of the list unreachable
expect(screen.queryByTestId('prompt-scroll-view')).toBeNull();
expect(screen.getByText(prompt)).toBeOnTheScreen();
});
}); |
…sts to unit suite Co-authored-by: TaduJR <TaduJR@users.noreply.github.com>
|
I made both test changes and pushed them in fe95be9.
I used your two cases as written, so the old "no provider" case is gone. Let me know if you want it back. Checks
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-Native.movAndroid: mWeb ChromeAndroid-mWeb.moviOS: HybridAppiOS-Native.mp4iOS: mWeb SafariiOS-Safari.mp4MacOS: Chrome / Safari |
|
Please make the PR ready for review. |
|
I couldn't mark this PR ready for review. The command to change the draft status was blocked in my environment, so the PR is still a draft. CI is green except |
|
cc @lakchote |
@lakchote I think Melvin couldn't make the PR ready |
|
Please make the PR ready. I already approved the PR as C+ |
|
I still can't mark this PR ready for review. My environment blocks the command that changes a PR's draft status, so the PR is still a draft. Retrying won't help until that permission changes.
All CI checks pass except |
|
Please make the PR ready. |
|
I couldn't mark this PR ready for review. My environment still blocks the command that changes a PR's draft status. Retrying won't help until someone changes that permission.
All CI checks pass except |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@dylanexpensify everything looks good from a product perspective? |
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
Looks good from a product perspective 👍
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.95-0 🚀
|
|
No help site changes are needed for this PR, so I didn't create a docs PR. This PR fixes how confirmation panels scroll on a phone held sideways (landscape). It changes no feature names, settings, buttons, or workflow steps. No article under |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.95-4 🚀
|
Explanation of Change
On a phone in landscape, confirmation modals open as a bottom sheet capped at 75% of the screen height, and
BaseModalwraps their content in a ScrollView so it can scroll. The bulk Submit and Export confirmations also put their list of report names in its own ScrollView. That left one scrolling area inside another, and iOS and Android disagree about which one gets the swipe:This change makes sure there is only ever one scrolling area.
BaseModalnow passes down, throughModalContext, whether it has wrapped its content in a ScrollView. When it has,ConfirmContentdoesn't add its own ScrollView around the report list.Only the three confirmations that show a scrollable report list are affected: blocked bulk Submit, partial Export and Export again. Other confirmation modals, tablets and desktop are unchanged.
Fixed Issues
$ #101333
PROPOSAL: #101333 (comment)
Tests
Prerequisite:
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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