Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the focus on Edit Box after edit action. #3441

Merged
merged 10 commits into from
Jun 11, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jun 8, 2021

Details

  1. Remove modal visibility status from the Context Menu. which is needed to make the edited comment as a prime actor for focus.
  2. Delayed the edit action after the Context menu is completely hidden which let us handle the focus. It removes the need for the SetTimeout hack.
  3. Set the focus on the Edit box after each edit action.
  4. Move the focus back to the main Composer when the Edit box is closed.

Fixed Issues

Fixes #3017
fixes #3309

Tests | QA Steps (Web|Desktop)

  • Sign in to the e.chat staging web app
  • Right-click the text and choose Edit Comment
  • Textbox should automatically get focused.
  • Click Edit comment from Hover menu. Edit box should automatically get focused.
  • Send a message, then hit the ArrowUp key. Edit box of your latest message should automatically get focused.
  • Hit the Esc key, and the main ReportActionCompose should regain focus.

Tests | QA Steps(mWeb|Android|IOS)

  • Sign in to the e.chat staging web app
  • Hold press any text message sent by you and choose Edit Comment
  • Textbox should automatically get focused.

Tests | QA Steps (Web|Desktop) for #3309

  • Sign in to the e.chat staging web app
  • Right-click the text and choose Edit Comment
  • Textbox should automatically get focused.
  • Click Edit comment from Hover menu. Edit box should automatically get focused.
  • Save changes , main chat composer should regain focus.
  • cancel Edit, main chat composer should regain focus.
  • Click edit comment for any message.
  • Hit the Esc key, and the main chat composer should regain focus.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

focus-edit-w.mp4

Mobile Web

1623176819336.mp4

Desktop

focus-edit-d.mp4

iOS

Android

1623177379471.mp4

@parasharrajat parasharrajat marked this pull request as ready for review June 8, 2021 18:28
@parasharrajat parasharrajat requested a review from a team as a code owner June 8, 2021 18:28
@MelvinBot MelvinBot requested review from AndrewGable and removed request for a team June 8, 2021 18:28
@parasharrajat parasharrajat changed the title Set the focus on Edit Box after edit action. [WIP] Set the focus on Edit Box after edit action. Jun 8, 2021
@AndrewGable AndrewGable requested review from deetergp and removed request for AndrewGable June 8, 2021 21:44
@parasharrajat parasharrajat changed the title [WIP] Set the focus on Edit Box after edit action. Set the focus on Edit Box after edit action. Jun 9, 2021
@parasharrajat
Copy link
Member Author

Ready to roll. Thanks.

@roryabraham
Copy link
Contributor

@parasharrajat Looks like lint is failing

@parasharrajat
Copy link
Member Author

@roryabraham this is due to #3401. And the fix is being made here #3481. So I suggest we merge it quickly.

@roryabraham
Copy link
Contributor

Lint error resolved, please merge main into this branch

@roryabraham
Copy link
Contributor

Also @parasharrajat will this also fix this deploy blocker?

@roryabraham
Copy link
Contributor

Also @parasharrajat will this also fix this deploy blocker?

Confirmed on iOS safari that it does.

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Also, I must be going crazy because I can't find where the isFocused prop of the ReportActionCompose comes from, because it isn't coming from here. Are we sure that this is ever being actually set? Can we safely get rid of it? We also have isFocused in the ReportActionCompose state.

src/components/Modal/ModalPropTypes.js Outdated Show resolved Hide resolved
src/libs/ReportActionComposeFocusManager.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionItem.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

IsFocused prop is coming from withNavigation HOC. It's the most important prop. Removing it is not an option. 😄

@parasharrajat
Copy link
Member Author

parasharrajat commented Jun 9, 2021

That deploy blocker is due to app crashes. It's not covered in it. Actually, I don't know the reason for the crash so far, thus It's hard to tell.

@parasharrajat
Copy link
Member Author

Updated. Thanks.

@parasharrajat
Copy link
Member Author

@deetergp @roryabraham Awaiting your feedback. Thanks.

@deetergp
Copy link
Contributor

Testing

Web

Editing via the context menu always puts focus to the edit text box, but the right-click > edit only does it on the first edit after a page load. In subsequent right-click > edits, the focus stays at the bottom, in the main text editor.

Desktop

Context menu Edit works like it should, but I couldn't get right-click > edit to focus in the edit text box, it always went down to the main text box.

mWeb

Works as intended 👍

iOS

Works as intended, though it's weird that you only get the long-press context menu if you click on your own avatar. That's probably unrelated.

@parasharrajat
Copy link
Member Author

@deetergp Thanks for letting me know. I changed the prop name but forgot to update it in the Component. shouldSetModalVisibility was the prop. Just fixed it. You can check now. Thanks.

@deetergp
Copy link
Contributor

That fix did it, thanks @parasharrajat 👍

Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

Code looks good and tests pass.

@parasharrajat
Copy link
Member Author

:shipit:

@parasharrajat
Copy link
Member Author

@roryabraham Any feedback here? I think this PR is waiting to be merged by you. 😅

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

This looks good to me. Sorry to have been a bottleneck here @parasharrajat – been a crazy couple days.

@roryabraham roryabraham merged commit 34612d7 into Expensify:main Jun 11, 2021
@OSBotify
Copy link
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
Contributor

🚀 Deployed to staging in version: 1.0.68-5🚀

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

@kavimuru
Copy link

iPhone - Safari - Cursor focus not gained for edit comment field

Expected Result:

Textbox should automatically get focused.

Actual Result:

Focus not gained in edit box

Actions Performed:

  1. Launch the URL and login
  2. Hold press any text message sent by you and choose Edit Comment

Platform:

iOS
mWeb ✔️
Android

Build:

1.0.69-0

Notes/Images/Video:

Bug5114863_RPReplay_Final1623793918.mp4

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label Jun 15, 2021
@parasharrajat
Copy link
Member Author

No worries I will test it on safari M-web

@roryabraham
Copy link
Contributor

I am unable to reproduce on iOS 14.5 Safari. @isagoico @kavimuru can we please retest this in the latest staging version and see if it has been resolved?

@parasharrajat
Copy link
Member Author

Same here issue is not reproducible on M-web safari iPhone 12 - 14.4.

@isagoico
Copy link

Retested and it was a pass in mWeb! Checking it off.

@isagoico isagoico removed the DeployBlockerCash This issue or pull request should block deployment label Jun 18, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants