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

[HOLD - App #26239] [$500] Mobile-Safari Auto Focuses Composer in Report on Existing chat #29509

Closed
1 of 6 tasks
m-natarajan opened this issue Oct 12, 2023 · 46 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Oct 12, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.3.83-5
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
Expensify/Expensify Issue URL:
Issue reported by: @jeet-dhandha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697136130670869

Action Performed:

  1. Open any Chat (which has some messages) in Mobile Safari.
  2. Verify Composer is Auto-focused.
  3. Test on iOS, Android, Mobile-Chrome for Step 1, Verify that it doesn't auto-focus on chat with messages.

Expected Result:

Composer in Mobile safari should not get auto-focused.

Actual Result:

Composer in Mobile-Safari get's auto-focused.

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Android: Native
focus-mweb-chrome.mov
Android: mWeb Chrome
iOS: Native
focus-iOS.mov
iOS: mWeb Safari
focus-mweb-safari.mov
RPReplay-Final1697147908.MP4
MacOS: Chrome / Safari
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d1e885de54d568ec
  • Upwork Job ID: 1712590372450942976
  • Last Price Increase: 2023-10-12
@m-natarajan m-natarajan added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 12, 2023
@melvin-bot melvin-bot bot changed the title Mobile-Safari Auto Focuses Composer in Report on Existing chat [$500] Mobile-Safari Auto Focuses Composer in Report on Existing chat Oct 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01d1e885de54d568ec

@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

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

@graylewis
Copy link
Contributor

graylewis commented Oct 12, 2023

I'm unable to replicate this on my iOS simulator on production or staging

@jeet-dhandha
Copy link
Contributor

jeet-dhandha commented Oct 13, 2023

Proposal

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

  • Currently mobile safari is focusing on Composer even if the shouldAutoFocus flag is false. Other mobile devices such as iOS, Android and mWeb Chrome does not have this issue.
  • We aren't focusing composer as user might just want to read the existing conversation.

What is the root cause of that problem?

  • For mWeb-Safari we have below code which handles the focus issue.

// For mobile Safari, updating the selection prop on an unfocused input will cause it to automatically gain focus
// and subsequent programmatic focus shifts (e.g., modal focus trap) to show the blue frame (:focus-visible style),
// so we need to ensure that it is only updated after focus.
const isMobileSafari = Browser.isMobileSafari();

const [selection, setSelection] = useState(() => ({
start: isMobileSafari && !shouldAutoFocus ? 0 : value.length,
end: isMobileSafari && !shouldAutoFocus ? 0 : value.length,
}));

  • But ComposerWithSuggestion also uses a Component called Composer whose index.js also has selection state which is causing the issue.

const [selection, setSelection] = useState({
start: initialValue.length,
end: initialValue.length,
});

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

  • If we update Composer with the code mentioned above in ComposerWithSuggestion then it will fix the issue.
  • And instead of shouldAutoFocus we will use autoFocus prop.
  • Also after this PR is merged,
  • we will need to update index.js of UpdateMultilineInputRange by adding below code which will disable unwanted selection, as updateMultilineInputRange will be called each time Composer is mounted.
export default function updateMultilineInputRange(input, shouldAutoFocus = true) {
    if (!input) {
        return;
    }

    if (input.value && input.setSelectionRange) {
        const length = input.value.length;
+       // To disable unwanted focus on Composer when `shouldAutoFocus` is false.
+       if (!shouldAutoFocus && Browser.isMobileSafari()) {
+          // eslint-disable-next-line no-param-reassign
+          input.scrollTop = input.scrollHeight;
+          return;
+       }

        input.setSelectionRange(length, length);
        // eslint-disable-next-line no-param-reassign
        input.scrollTop = input.scrollHeight;
    }
}

What alternative solutions did you explore? (Optional)

  • N/A

@jeet-dhandha
Copy link
Contributor

Before Fix:

Screen.Recording.2023-10-13.at.9.17.45.AM.mov

After Fix (Including above mentioned PR changes):

Screen.Recording.2023-10-13.at.9.14.52.AM.mov

@parasharrajat
Copy link
Member

Feels like we have a dependency on #28790 for above proposal.

@alexpensify
Copy link
Contributor

@parasharrajat - are you suggesting that we should move this to be on HOLD until that GH is resolved?

@parasharrajat
Copy link
Member

No, there might be more proposals.

@jeet-dhandha
Copy link
Contributor

@parasharrajat My solution will work in current main branch but if we merge the mentioned PR, then the issue will rise again and will then be counted as regression. So to avoid that i mentioned it here.

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@parasharrajat

This comment was marked as outdated.

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

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

@thienlnam
Copy link
Contributor

I think we should hold all these for #26239 which should ideally fix the root cause for all these focus issues upstream

@alexpensify alexpensify changed the title [$500] Mobile-Safari Auto Focuses Composer in Report on Existing chat [HOLD - App #26239] [$500] Mobile-Safari Auto Focuses Composer in Report on Existing chat Oct 17, 2023
@alexpensify alexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 17, 2023
@alexpensify
Copy link
Contributor

Cool, moving this to weekly for now

@jeet-dhandha
Copy link
Contributor

Also #28790 this PR is merged so we can move on with this issue.

@thienlnam
Copy link
Contributor

I wouldn't consider this a severe enough bug to need to get a fix out immediately - let's just wait for the upstream fix so we don't have to have a bunch of back and fourths

@alexpensify
Copy link
Contributor

Monthly Update: We are at 17.3.1. Setting a reminder to check in next month.

@alexpensify
Copy link
Contributor

@ntdiary - I believe that we are at Version 17.4.1 now, can we confirm if we can take this one off hold? Thanks!

@ntdiary
Copy link
Contributor

ntdiary commented Apr 4, 2024

@ntdiary - I believe that we are at Version 17.4.1 now, can we confirm if we can take this one off hold? Thanks!

Yeah, our upstream PR has been released in 17.4. :)

@alexpensify alexpensify added Daily KSv2 and removed Monthly KSv2 labels Apr 9, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 9, 2024
@alexpensify
Copy link
Contributor

We are back open for proposals here. Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Apr 9, 2024
@parasharrajat
Copy link
Member

@alexpensify Could you please remove the Hold from the title and reconfirm details in OP? It been so long, it might be outdated.

@parasharrajat
Copy link
Member

@Expensify/applause can you please recheck this issue? Is it still valid based on the latest regression steps?

@alexpensify alexpensify added Weekly KSv2 and removed Daily KSv2 labels Apr 11, 2024
@alexpensify
Copy link
Contributor

I'm going to put this back on hold for #26239. It looks like there will be some cleanup in the other GH.

@alexpensify
Copy link
Contributor

Weekly Update: The other GH is going through a clean-up process.

@alexpensify
Copy link
Contributor

Weekly Update: The other GH is still being updated

@alexpensify
Copy link
Contributor

Weekly Update: The other GH is on hold for another update

@alexpensify
Copy link
Contributor

Weekly Update: Same as last week, waiting on the other GH.

@alexpensify
Copy link
Contributor

Weekly Update: #26239 is moving along, we can test this one again once it's in Staging or production.

@alexpensify
Copy link
Contributor

Heads up, I will be offline until Tuesday, May 28, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

@melvin-bot melvin-bot bot added the Overdue label May 27, 2024
@alexpensify
Copy link
Contributor

#26239 is in production. @Expensify/applause can you please recheck this issue? Is it still valid based on the latest regression steps? Thanks!

cc: @ntdiary who worked on the PR above.

@melvin-bot melvin-bot bot removed the Overdue label May 28, 2024
@alexpensify
Copy link
Contributor

Weekly Update: Waiting for a retest.

@alexpensify
Copy link
Contributor

Weekly Update: I need to bump the Slack room, no movement here yet for the retest

@alexpensify
Copy link
Contributor

Weekly Update: Waiting on a retest.

@parasharrajat
Copy link
Member

It has been quite a long time waiting for retesting. @alexpensify Could you please help to push this forward? I see no use in keeping this issue open.

@m-natarajan
Copy link
Author

Issue is not reproducible anymore.

RPReplay_Final1718808213.MP4

@alexpensify
Copy link
Contributor

Already did @parasharrajat. 😄

Thank you everyone for the help here. I'm going to close since the other GH fixed this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants