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

IOS - Assign task - Text is flickering in case of editing saved description field #20836

Closed
1 of 6 tasks
kbecciv opened this issue Jun 15, 2023 · 24 comments
Closed
1 of 6 tasks
Assignees

Comments

@kbecciv
Copy link

kbecciv commented Jun 15, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when executing PR #19377

Action Performed:

  1. Assign a new task, fill title, multiline description and tap on the Next button
  2. Try to edit multiline description field

Expected Result:

The user should be able to edit description fields without any issues

Actual Result:

Text is flickering in case of editing multi line description field

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.28.3

Reproducible in staging?: yes

Reproducible in production?: new feature

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6094308_RPReplay_Final1686841212_2.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jun 15, 2023
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Jun 15, 2023

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@dan1d

This comment was marked as off-topic.

@thienlnam
Copy link
Contributor

@dan1d Please bring any setup related questions to #expensify-open-source in slack!

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jun 15, 2023
@thienlnam
Copy link
Contributor

Not a blocker, only happens when you go beyond the multiple lines. This appears to be a regression of this though

#19135

@akinwale
Copy link
Contributor

akinwale commented Jun 15, 2023

Looks like this is a side effect of using the state to set the TextInput selection prop. It's happening in the NewTaskDescriptionPage and TaskDescriptionPage components. This didn't seem to happen during my tests so I may have missed it somehow.

After taking a look, I can use the onEntryTransitionEnd event handler to set the selection range after inputRef.current.focus(), like so:

if (inputRef.value && inputRef.setSelectionRange) {
    const length = inputRef.value.length;
    inputRef.setSelectionRange(length, length);
}

This is the way it's being done for the welcomeMessage text input in WorkspaceInviteMessagePage. It places the text cursor at the end, however, the text input does not automatically scroll to the position until typing starts. I believe the underlying issues with the multiline TextInput control would have to be handled in a separate issue for this to work properly.

@sobitneupane What do you think of this approach?

@sobitneupane
Copy link
Contributor

This is the way it's being done for the welcomeMessage text input in WorkspaceInviteMessagePage. It places the text cursor at the end, however, the text input does not automatically scroll to the position until typing starts.

@akinwale We want the text to be scrolled as well.

@akinwale
Copy link
Contributor

This is the way it's being done for the welcomeMessage text input in WorkspaceInviteMessagePage. It places the text cursor at the end, however, the text input does not automatically scroll to the position until typing starts.

@akinwale We want the text to be scrolled as well.

@sobitneupane Apparently, using the selection prop on iOS appears to be a bug with React Native.

I'll try to find another solution that works, hopefully with the scrolling working properly too.

@akinwale
Copy link
Contributor

@sobitneupane

I've spent quite a number of hours on this, and it appears that the TextInput just won't scroll to the end without user interaction. Every attempt that I've made to programmatically achieve this does not change the scroll position.

I have tried the following:

  • Tried the setSelectionRange as indicated above, but apparently, this will only work on web, not native. The cursor is already always at the end for native platforms.
  • Used setNativeProps to set the selection, however, this does not make the scroll position change.
  • Tried to set the selection prop to undefined after initially setting it. This will still reset the cursor position back to the start of the TextInput.
  • Looked for a way to scroll the TextInput directly. Unfortunately, it looks like this is not possible as there are no functions exposed to do this.
  • Tried to programmatically dispatch a keydown event for the Down cursor key. This can be achieved on web platforms using KeyboardEvent, but I'm not seeing a way to do this on native.

I'd like to attempt wrapping the TextInput with a ScrollView as I've seen in some suggestions, which would allow being able to programmatically scroll the ScrollView using the scrollToEnd method, but we may lose the bottom border styling on the text input. Please let me know what you think of this approach.

If you also have any other ideas that may help here, please let me know so that I can try. Thanks.

@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2023

@akinwale, @sobitneupane, @thienlnam Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@sobitneupane
Copy link
Contributor

@akinwale Did we find any better solution that won't cause any regression?

@melvin-bot melvin-bot bot removed the Overdue label Jun 21, 2023
@akinwale
Copy link
Contributor

@sobitneupane I haven't found any. I experimented with wrapping the TextInput with a ScrollView but this breaks the input styling and also requires a fixed height to be set, so it doesn't autogrow.

Right now, the viable options I can see in order to move forward are:

  1. Remove the selection prop and use the ref solution proposed above so that it doesn't cause issues on iOS. This however means that the user would have to manually scroll to the bottom (only on iOS native) when trying to edit.
  2. Leave the selection prop as is and hope it gets fixed on the React Native side.

Please let me know what you think. Thanks.

@sobitneupane
Copy link
Contributor

Leave the selection prop as is and hope it gets fixed on the React Native side.

Is there any open issue for this on React Native?

@akinwale
Copy link
Contributor

Leave the selection prop as is and hope it gets fixed on the React Native side.

Is there any open issue for this on React Native?

Yes, since 2020. facebook/react-native#30298

@sobitneupane
Copy link
Contributor

Oh no. We cannot wait for the issue that is open from 2020.😄

@sobitneupane
Copy link
Contributor

Remove the selection prop and use the ref solution proposed above so that it doesn't cause issues on iOS. This however means that the user would have to manually scroll to the bottom (only on iOS native) when trying to edit.

So, the best solution we have as of now is #20836 (comment)

What's your opinion on this @thienlnam?

@thienlnam
Copy link
Contributor

thienlnam commented Jun 21, 2023

Thank you both for your investigation into this - let's continue with this proposal to fix the regression for now #20836 (comment)

But since there is an issue in RN for this, let's also try to get some proposals to resolve that root cause over there as well.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jun 23, 2023
@akinwale
Copy link
Contributor

@sobitneupane The PR is ready for review. As can be observed from my test videos, the description input automatically scrolls to the bottom when editing for every platform except iOS native.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jun 29, 2023
@melvin-bot melvin-bot bot changed the title IOS - Assign task - Text is flickering in case of editing saved description field [HOLD for payment 2023-07-06] IOS - Assign task - Text is flickering in case of editing saved description field Jun 29, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.34-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-07-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Jul 9, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

@akinwale, @sobitneupane, @thienlnam Huh... This is 4 days overdue. Who can take care of this?

@thienlnam
Copy link
Contributor

No payment due here since we fixed a regression

@melvin-bot melvin-bot bot removed the Overdue label Jul 11, 2023
@thienlnam thienlnam changed the title [HOLD for payment 2023-07-06] IOS - Assign task - Text is flickering in case of editing saved description field IOS - Assign task - Text is flickering in case of editing saved description field Jul 11, 2023
@thienlnam thienlnam added Weekly KSv2 and removed Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Jul 11, 2023
@thienlnam
Copy link
Contributor

Going to close this for now, I'd like to revisit facebook/react-native#30298 in the future to see if we can find someone to make a proposal for that

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

No branches or pull requests

6 participants