[HOLD for payment 2023-07-21] [$1000] Room settings - Edit room name tab is cut if edit and re-open it

[lanitochka17]

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

---

Action Performed:

1. Open https://staging.new.expensify.com/r/8721544721767866
2. Open a room chat
3. Click on room header to open "details"
4. Click on "Settings"
5. Click on "Room name"
6. Edit room name
7. Click on "Save"
8. Click on "Room name"
9. Repeat steps "6-8" several more times if the issue doesn't reproduce

Expected Result:

"Edit room name" tab is completely visible after opening

Actual Result:

"Edit room name" tab is shifted to the left and is partially cut
Issue is both reproducible offline and online

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.36.4

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6117297_Recording__20.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010b1530b30a801399
• Upwork Job ID: 1676716588882231296
• Last Price Increase: 2023-07-05

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[trjExpensify]

Yup, this is reproducible but only on Web Safari for me:

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010b1530b30a801399

[melvin-bot]

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[b4s36t4]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Room settings - Edit room name tab is cut if edit and re-open it

What is the root cause of that problem?

Edit Room name page contains an Input which is auto-focusing itself.
The Input is getting autofoucs without the completion of page rendering which is the main reason for this.
I see In safari the transition is getting half ended because of auto-focus. If you observe in chrome/firefox there won't be any transition cause of auto foucs.

App/src/pages/settings/Report/RoomNamePage.js

Line 85 in 7036597

autoFocus

This autoFocus prop causing the issue.

What changes do you think we should make in order to solve the problem?

We should delay the auto-focus so that by then Edit room page is full rendered.

Changes Needed

Add shouldDelayFocus prop inside RoomNamePage.js and Pass shouldDelayFocus prop to BaseInput inside RoomNameInput.js

Here ->

App/src/components/RoomNameInput/index.js

Line 62 in 41eb9ba

<TextInput

Fix Implementation Demo:

iuu.mp4

What alternative solutions did you explore? (Optional)

NA

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

"Edit room name" tab is shifted to the left and is partially cut

What is the root cause of that problem?

The root cause is that the room name input is auto focused in the middle of screen transition. This will cause the transition to pause on Safari.

What changes do you think we should make in order to solve the problem?

We need to focus the input after the screen transition already completes, similar to what we've done through out the app.

1. Define a ref in RoomNamePage

const roomNameInput = useRef(null);

2. Remove autofocus and add the input ref here

   App/src/pages/settings/Report/RoomNamePage.js

   Line 85 in 0e21bc5

   autoFocus

ref={(el) => (roomNameInput.current = el)}

3. Add onEntryTransitionEnd here

onEntryTransitionEnd={() => roomNameInput.current && roomNameInput.current.focus()}

Some examples of this approach being used through out the app: here, here and here.
We should not use shouldDelayFocus because it's a hack that use timeout, also if the user access the page directly via link, they're always kept waiting 300ms before the input is focused.

Update: There's an issue a small issue with the above approach where if we access the page via link, sometimes the input is not focused, this is due to the roomNameInput.current might not be initialized yet at the time onEntryTransitionEnd is called. To fix this we can simply introduce add the .focus() in an useEffect so that it will only focus if both screenTransitionEnd is true and roomNameInput.current is defined.

useEffect(() => {
        if (screenTransitionEnd && roomNameInput.current) {
              roomNameInput.current.focus();
        }
    }, [screenTransitionEnd, roomNameInput.current]);

What alternative solutions did you explore? (Optional)

This might be happening in other places so we should check as well.

[trjExpensify]

@Santhosh-Sellavel let us know what you think!

We should not use shouldDelayFocus because it's a hack that use timeout.

❤️

[b4s36t4]

I feel autofocus and shouldDelay props are been there because we don't have to mess with the ref. If we want to take control of the focusing of an input then I feel autoFocus prop doesn't have a meaning. Maybe we should consider a singular approach meaning to go with only one way.

[tienifr]

@b4s36t4 shouldDelayFocus is actually the legacy approach, for new components we have moved to use the onEntryTransitionEnd approach since it's more robust. You can see here for explanation.

[hoangzinh]

fyi, I think onEntryTransitionEnd is a good approach, but it won't work when we open the page by direct link.

[b4s36t4]

@hoangzinh yes if I refresh the page input is not getting auto focus. Tried below method [Image 1] it did worked out pretty well in foucinsg. But it did thrown an error [Image 2]. Maybe going with shouldDelayFocus?

[Santhosh-Sellavel]

@trjExpensify Can you assign a new C+ to take over this, as I'm low on bandwidth, thanks!

[trjExpensify]

Sure! It's good to ask in C+ for someone else to take over btw. Done that here.

[melvin-bot]

📣 @allroundexperts 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

fyi, I think onEntryTransitionEnd is a good approach, but it won't work when we open the page by direct link.

@hoangzinh nice catch! For that we can add a simple fix to make sure the ref exists at the time of the focus.
Proposal updated for that

[allroundexperts]

Thanks for your proposal @b4s36t4 but as correctly pointed out by @tienifr, we avoid using shouldDelayFocus because it uses timeouts.
@tienifr's proposal looks good to me. I agree that the issue pointed out by @hoangzinh might be present at other places where we use a similar approach and might be worth checking / fixing as well.

🎀 👀 🎀 C+ reviewed

[tienifr]

@chiragxarora Yup, but we have decided to remove that part as explained in #22621 (comment)

[chiragxarora]

hmmm fair

[melvin-bot]

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

[melvin-bot]

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

• Fix: Room name input blocks screen transition #22621

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

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]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[bfitzexpensify]

Seems like #22973 might be a regression caused by the fix here - finding that the room name isn't brought into focus when using a deep link to access this settings page. No issue in Chrome but I can reliably reproduce in Safari cc @tienifr

[allroundexperts]

Seems like #22973 might be a regression caused by the fix here - finding that the room name isn't brought into focus when using a deep link to access this settings page. No issue in Chrome but I can reliably reproduce in Safari cc @tienifr

@bfitzexpensify We actually discussed this case here. This used to happen before our PR as well.

[bfitzexpensify]

Nice, thanks for the context @allroundexperts

[allroundexperts]

The checklist is not needed here. This is a general issue found throughout our App where we focus inputs without waiting for transitions to end. Seems like we forgot to take into account the page transitions when auto focusing inputs. We are actively fixing all other instances of this issue as well.

[trjExpensify]

Thanks, @allroundexperts!

Seems like we forgot to take into account the page transitions when auto focusing inputs. We are actively fixing all other instances of this issue as well.

Just on this. Do you think it could be avoided again in the future though, and perhaps be worthwhile documenting somewhere to make sure we don't forget going forward?

[allroundexperts]

I think we can add this into our Form guidelines. But in order to do that, we might need a consensus on what's the best way to focus an input. Right now, we're using shouldDelayFocus, InteractionManager.runAfterInteractions and onTransitionEnd property to trigger the focus.

[trjExpensify]

Confirming payment offers sent:

• $1,500 - @tienifr for the fix + #urgency bonus
• $1,500 - @allroundexperts for the C+ review + #urgency bonus

[trjExpensify]

I think we can add this into our Form guidelines. But in order to do that, we might need a consensus on what's the best way to focus an input. Right now, we're using shouldDelayFocus, InteractionManager.runAfterInteractions and onTransitionEnd property to trigger the focus.

Okay, so let's maybe have that discussion? Otherwise I think this is likely just going to crop up again for the next time we add a form etc.

[allroundexperts]

I think we can add this into our Form guidelines. But in order to do that, we might need a consensus on what's the best way to focus an input. Right now, we're using shouldDelayFocus, InteractionManager.runAfterInteractions and onTransitionEnd property to trigger the focus.

Okay, so let's maybe have that discussion? Otherwise I think this is likely just going to crop up again for the next time we add a form etc.

Sure. I'll create one!

[allroundexperts]

Posted here

[allroundexperts]

Also, can you please withdraw the Upwork offer? I'll be getting paid through the app.

[anmurali]

Approved 1500 to Sibtain based on #22271 (comment)

[trjExpensify]

Also, can you please withdraw the Upwork offer? I'll be getting paid through the app.

I'm getting 404s when I try to access the offer page. I've gone ahead and closed the job though anyhow.

Posted here

Thanks, let's keep the discussion going there 👍

Everyone is paid, closing!