fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

aimane-chnaif · 2022-10-20T04:01:40Z

Details

In PopoverMenu component, props.onItemSelected(item); should be called first. And then item.onSelected(); should be called after onModalHide callback.
This is already done in native side: https://github.com/Expensify/App/blob/main/src/components/PopoverMenu/index.native.js
We no longer need platform specific handling for this component.
So remove current index.js and rename index.native.js to index.js

Fixed Issues

$ #10888
PROPOSAL: #10888 (comment)

Tests

Login with any account on desktop or web.
Click on any report in LHN.
Click on + icon on left end of composer.
Click one of Request money / Split money / Split bill.
Click cross icon to close this modal page.
Verify that composer input is focused automatically

NOTE: It's intended behavior to prevent focus on touchable devices (native, mWeb) after navigating back from modal screens. reference:

App/src/pages/home/report/ReportActionCompose.js

Lines 168 to 174 in fbf0334

    
           // We want to focus or refocus the input when a modal has been closed and the underlying screen is focused. 
        
           // We avoid doing this on native platforms since the software keyboard popping 
        
           // open creates a jarring and broken UX. 
        
           if (this.shouldFocusInputOnScreenFocus && this.props.isFocused 
        
               && prevProps.modal.isVisible && !this.props.modal.isVisible) { 
        
               this.focus(); 
        
           }

Verify that no errors appear in the JS console

QA Steps

Login with any account on desktop or web.
Click on any report in LHN.
Click on + icon on left end of composer.
Click one of Request money / Split money / Split bill.
Click cross icon to close this modal page.
Verify that composer input is focused automatically

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

web.mov

Mobile Web - Chrome

mchrome.mp4

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mp4

mananjadhav · 2022-10-27T22:29:50Z

@Luke9389 @aimane-chnaif I was just about to test and while checking the code locally, I think we can further refactor this.

We have BasePopoverMenu currently, only because we have two files, index.js and index.native.js. With us merging all code into index.js, BasePopoverMenu is redundant. I feel we should merge BasePopoverMenu itno PopoverMenu/index.js

What do you folks think?

aimane-chnaif · 2022-10-28T03:13:20Z

We have BasePopoverMenu currently, only because we have two files, index.js and index.native.js. With us merging all code into index.js, BasePopoverMenu is redundant. I feel we should merge BasePopoverMenu itno PopoverMenu/index.js

I agree

mananjadhav · 2022-11-01T14:29:05Z

@aimane-chnaif Can you implement the changes requested?

cc - @Luke9389

…o fix-10888

aimane-chnaif · 2022-11-01T16:36:01Z

@mananjadhav ready for review

mananjadhav · 2022-11-02T19:16:41Z

thanks @aimane-chnaif. Will need 1-2 days to test this as it'll required a good amount of regression suite to test.

mananjadhav · 2022-11-03T22:50:51Z

Thanks for the changes and patience on this one @aimane-chnaif. Changes look good and test well. I agree with your comment that for mobile devices (app and mweb) we don't want to refocus as it'll popup the keyboard.

@Luke9389 All yours

🎀 👀 🎀
C+ reviewed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- 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 shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
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(themeColors.componentBG)
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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web-refocus-composer.mov

Mobile Web - Chrome

mweb-chrome-dont-refocus-composer.mov

Mobile Web - Safari

mweb-safari-dont-refocus-composer.mov

Desktop

desktop-refocus-composer.mov

iOS

ios-dont-refocus-composer.mov

Android

android-dont-refocus-composer.mov

Luke9389

I'll be coming around to this one tomorrow. Thanks for the changes.

mananjadhav · 2022-11-08T04:44:11Z

@Luke9389 Quick bump on this one

mananjadhav · 2022-11-10T13:48:18Z

@Luke9389 did you get a chance to look at this PR?

Luke9389 · 2022-11-11T22:19:05Z

Oh man, missed it. Thanks for the bump(s)

Luke9389 · 2022-11-14T19:58:34Z

src/components/PopoverMenu/index.js

+        this.onMenuHide = () => {
+            item.onSelected();
+
+            // Clean up: open and immediately cancel should not re-trigger the last action


Where do we stand with this? Have we tested that?

I know this came from the previous code but now that we're changing this component we should be sure it works in all contexts.

This code is inside selectItem(item) to get item and call item.onSelected() after modal fully closed, which was selected before triggering modal close.
But I suggest to refactor this to use variable instead of function inside function:

constructor(props) { super(props); this.state = { focusedIndex: -1, }; this.updateFocusedIndex = this.updateFocusedIndex.bind(this); this.resetFocusAndHideModal = this.resetFocusAndHideModal.bind(this); this.removeKeyboardListener = this.removeKeyboardListener.bind(this); this.attachKeyboardListener = this.attachKeyboardListener.bind(this); - this.onMenuHide = () => {}; + this.selectedItem = null; }

- /** - * Set the item's `onSelected` action to fire after the menu popup closes - * @param {{onSelected: function}} item - */ selectItem(item) { - this.onMenuHide = () => { - item.onSelected(); - - // Clean up: open and immediately cancel should not re-trigger the last action - this.onMenuHide = () => {}; - }; + this.selectedItem = item; this.props.onItemSelected(item); }

resetFocusAndHideModal() { this.updateFocusedIndex(-1); // Reset the focusedIndex on modal hide this.removeKeyboardListener(); - this.onMenuHide(); + if (this.selectedItem) { + this.selectedItem.onSelected(); + this.selectedItem = null; + } }

Yeah I like that change. Go ahead and commit it and test it out!

Luke9389 · 2022-11-14T19:59:25Z

src/components/PopoverMenu/index.js

+    /**
+     * @param {Number} index
+     */
+    updateFocusedIndex(index) {
+        this.setState({focusedIndex: index});
+    }


I don't think this should be it's own function. Just use setState when you need it.

src/components/PopoverMenu/index.js

…o fix-10888

aimane-chnaif · 2022-11-16T22:29:12Z

@Luke9389 ready for review

Luke9389 · 2022-11-16T23:11:31Z

@aimane-chnaif did you test this on all platforms (multiple pop-overs, not just the bill split one)?

aimane-chnaif · 2022-11-16T23:49:22Z

@aimane-chnaif did you test this on all platforms (multiple pop-overs, not just the bill split one)?

yes, tested all Popover modals and accessibility

mananjadhav · 2022-11-22T11:36:49Z

@Luke9389 @aimane-chnaif Is there anything pending here? Then I can retest with the latest changes and update the checklist again.

aimane-chnaif · 2022-11-22T11:38:13Z

@Luke9389 @aimane-chnaif Is there anything pending here? Then I can retest with the latest changes and update the checklist again.

yes please. tests well on my side

mananjadhav · 2022-11-22T17:39:14Z

I've tested again with the updated changes. Thanks for the changes @aimane-chnaif.

@Luke9389 All yours.

🎀 👀 🎀
C+ reviewed

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- 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 shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
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(themeColors.componentBG)
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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

web-refocus-composer.mov

Mobile Web - Chrome

mweb-chrome-refocus-compoer.mov

Mobile Web - Safari

mweb-safari-refocus-compoer.mov

Desktop

desktop-refocus-composer.mov

iOS

ios-refocus-compoer.mov

Android

android-refocus-composer.mov

mananjadhav · 2022-11-27T06:36:08Z

@Luke9389 Did you get a chance to look at the previous comments?

mananjadhav · 2022-11-29T18:48:45Z

@Luke9389 This PR is blocked by your review and approval.

OSBotify · 2022-12-07T00:45:17Z

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

marcaaron · 2022-12-13T22:30:39Z

Hi this PR has been linked to a regression here: #13435

I think since this is a lower level change more care could have been applied to test all the existing popovers. In fact, that is what this checkbox is asking us to do.

I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)

@mananjadhav @aimane-chnaif please keep this in mind in the future. If we are making changes to something that is used multiple places it's a good idea to quickly check all the usages of that thing, thanks!

OSBotify · 2022-12-13T22:39:32Z

🚀 Deployed to production by @chiragsalian in version: 1.2.38-6 🚀

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

aimane-chnaif added 2 commits October 20, 2022 13:12

fix "Closing split bill doesn't refocus on composer" on web/desktop

e89959f

remove PopoverMenu native file

8acc26b

aimane-chnaif requested a review from a team as a code owner October 20, 2022 04:01

melvin-bot bot requested review from Luke9389 and mananjadhav and removed request for a team October 20, 2022 04:01

arielgreen mentioned this pull request Nov 1, 2022

[HOLD for payment 2022-12-20] [$250] Closing split bill doesn't refocus on text-input(composer) if already focused - reported by @aneequeahmad #10888

Closed

aimane-chnaif added 3 commits November 2, 2022 02:11

refactor PopoverMenu component

36e4e73

Merge branch 'main' of https://github.com/aimane-chnaif/Expensify int…

8bd3fce

…o fix-10888

remove BasePopoverMenu component

94fca97

Luke9389 reviewed Nov 4, 2022

View reviewed changes

Luke9389 suggested changes Nov 14, 2022

View reviewed changes

aimane-chnaif added 2 commits November 17, 2022 06:18

Merge branch 'main' of https://github.com/aimane-chnaif/Expensify int…

44c678b

…o fix-10888

refactor PopoverMenu component

f9488ac

mananjadhav approved these changes Nov 22, 2022

View reviewed changes

Luke9389 approved these changes Dec 7, 2022

View reviewed changes

Luke9389 assigned Luke9389 and aimane-chnaif and unassigned Luke9389 Dec 7, 2022

Luke9389 merged commit 28851c9 into Expensify:main Dec 7, 2022

This was referenced Dec 8, 2022

Deploy Checklist: New Expensify 2022-12-07 #13431

Closed

[HOLD for payment 2022-12-20] [$1000] Profile : Upload/Delete Photo popup not hiding #13435

Closed

allroundexperts mentioned this pull request Feb 13, 2023

[HOLD for payment 2023-03-09] [$4000] Opening search using the keyboard shortcut (ctrl+k) while options for adding payment method are visible, options overlap with search. #14889

Closed

1 task

s77rt mentioned this pull request Apr 15, 2023

[$250] Safari mWeb - Keyboard won't open for some inputs (New Chat, Change Password, New Room) #10414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

aimane-chnaif commented Oct 20, 2022 •

edited

Loading

mananjadhav commented Oct 27, 2022

aimane-chnaif commented Oct 28, 2022

mananjadhav commented Nov 1, 2022

aimane-chnaif commented Nov 1, 2022

mananjadhav commented Nov 2, 2022

mananjadhav commented Nov 3, 2022

Luke9389 left a comment

mananjadhav commented Nov 8, 2022

mananjadhav commented Nov 10, 2022

Luke9389 commented Nov 11, 2022

Luke9389 Nov 14, 2022

aimane-chnaif Nov 15, 2022 •

edited

Loading

Luke9389 Nov 16, 2022

Luke9389 Nov 14, 2022

aimane-chnaif commented Nov 16, 2022

Luke9389 commented Nov 16, 2022

aimane-chnaif commented Nov 16, 2022

mananjadhav commented Nov 22, 2022

aimane-chnaif commented Nov 22, 2022

mananjadhav commented Nov 22, 2022

mananjadhav commented Nov 27, 2022

mananjadhav commented Nov 29, 2022

OSBotify commented Dec 7, 2022

marcaaron commented Dec 13, 2022

OSBotify commented Dec 13, 2022

	// We want to focus or refocus the input when a modal has been closed and the underlying screen is focused.
	// We avoid doing this on native platforms since the software keyboard popping
	// open creates a jarring and broken UX.
	if (this.shouldFocusInputOnScreenFocus && this.props.isFocused
	&& prevProps.modal.isVisible && !this.props.modal.isVisible) {
	this.focus();
	}

fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

Conversation

aimane-chnaif commented Oct 20, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

mananjadhav commented Oct 27, 2022

aimane-chnaif commented Oct 28, 2022

mananjadhav commented Nov 1, 2022

aimane-chnaif commented Nov 1, 2022

mananjadhav commented Nov 2, 2022

mananjadhav commented Nov 3, 2022

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Luke9389 left a comment

Choose a reason for hiding this comment

mananjadhav commented Nov 8, 2022

mananjadhav commented Nov 10, 2022

Luke9389 commented Nov 11, 2022

Luke9389 Nov 14, 2022

Choose a reason for hiding this comment

aimane-chnaif Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Luke9389 Nov 16, 2022

Choose a reason for hiding this comment

Luke9389 Nov 14, 2022

Choose a reason for hiding this comment

aimane-chnaif commented Nov 16, 2022

Luke9389 commented Nov 16, 2022

aimane-chnaif commented Nov 16, 2022

mananjadhav commented Nov 22, 2022

aimane-chnaif commented Nov 22, 2022

mananjadhav commented Nov 22, 2022

Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

mananjadhav commented Nov 27, 2022

mananjadhav commented Nov 29, 2022

OSBotify commented Dec 7, 2022

marcaaron commented Dec 13, 2022

OSBotify commented Dec 13, 2022

aimane-chnaif commented Oct 20, 2022 •

edited

Loading

aimane-chnaif Nov 15, 2022 •

edited

Loading