[HOLD for payment 2024-07-24] [$250] Web - Sign in - Safari Password module does not go away after signing in

[kbecciv]

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

---

Version Number: 1.4.9.1
Reproducible in staging?: y
Reproducible in production?: y
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: Applause - Internal Team
Slack conversation:

Action Performed:

Pre-Cond: You have at least one login saved on Safari browser

1. Access staging.new.expensify.com on Safari
2. Sign into a valid account and on the password screen DON'T dismiss the password module
3. Input the magic code and navigate the app

Expected Result:

User expects two things, that either this password module doesn't show at all (Because now you need a magic code and its no longer relevant) OR that if it does show, its dismissed after signing in.

Actual Result:

The password module does not dismiss itself and the user navigates through the site and it continues to show

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

Add any screenshot/video evidence

Bug6304574_1701962100248.Password_module_continues_after_signing_in_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019a736d294b1b7be1
• Upwork Job ID: 1732796554278481920
• Last Price Increase: 2024-06-28
• Automatic offers:
• ikevin127 | Contributor | 102946366

Issue Owner

Current Issue Owner: @Christinadobrzyn

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019a736d294b1b7be1

[melvin-bot]

Triggered auto assignment to @MitchExpensify (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

[melvin-bot]

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

[ikevin127]

Proposal

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

The auto-fill suggestion popup is not dismissed after introducing magic code and remains stuck as the user navigates through the site.

What is the root cause of that problem?

If you use any <input> field without wrapping it in <form> tag, Safari 11+ or latest will show the auto-fill suggestion popup.

Our issue comes from the ...pages/signin/ValidateCodeForm/BaseValidateCodeForm.js component because we have no logic that handles the Safari auto-fill suggestion popup accordingly when it comes to the MagicCodeInput component where we don't actually need autofill logic because the magic code is randomly generated on every login.

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

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.tsx

Lines 293 to 295 in 633e708

	return (
	<>
	{/* At this point, if we know the account requires 2FA we already successfully authenticated */}

in the component mentioned above, instead of the react fragment <> ... </> add <form>...</form> to wrap the whole sign-in magic code input logic. This way the auto-fill suggestion popup will not be shown for the magic code input and our current logic won't be affected.

Videos

MacOS: Safari

dfa96d63-0dac-4533-bbf3-6c11acf2fee6.mp4

[tienifr]

Proposal

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

The password module does not dismiss itself and the user navigates through the site and it continues to show

What is the root cause of that problem?

According to this confirmation, we'll be using the RCA suggested here

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

According to this confirmation. We'll be using the solution suggested here

What alternative solutions did you explore? (Optional)

My original solution:

When we validate the code, in here, the blurring of the input happens at the same time as component rerendering due to code submission. This interferes with Safari's autofill popup showing and causes the autofill popup to be stuck on the screen.

We just need to wrap these lines inside InteractionManager.runAfterInteractions to make sure the blurring of the input happens after the component rendering is completed and doesn't interfere with Safari autofill popup.

[melvin-bot]

@MitchExpensify, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MitchExpensify]

You won't need a magic code to sign into NewDot soon, closing

[tienifr]

@MitchExpensify can you link the issue relating to the change that will remove the magic code signin? We might want to keep this issue on hold for that since it might still happen with the login method that replaces magic code

[MitchExpensify]

@tienifr #30794

[ikevin127]

You won't need a magic code to sign into NewDot soon, closing

@MitchExpensify Looks like this issue is still present 6 months later as we still need magic code to sign-in into ND.

I think this is worth fixing in the meantime until PR from issue #30794 is implemented, since it's an easy fix and more importantly it's really bad UX for both desktop and mobile Safari users (cc @Expensify/design). My proposal is still valid and ready for review / implementation if selected.

Note

Bumping on this since I still encounter the bug everytime I test on Safari and I need to login 😅
I can take this on fixing this as both Contributor and C+ reviewer if @situchan is not available for review.

[situchan]

I am still available for C+ review.

[shawnborton]

Cool, this is probably worth fixing but I think we should drastically reduce the bug bounty here.

[ikevin127]

Indeed, I agree based on what was stated in this Slack 🧵 regarding the bounty change from $500 to $250 that:

Any issues created before this date will maintain their original stated price.

but given that an issue is in the works to eventually remove the magic code functionality, I think $250 would be fair.

Do we agree 👍 or 👎.

[trjExpensify]

Yeah, I don't think unvalidated sign-ups makes a difference here. If at any point you decide to sign-out and then need to sign back in using the magic code this autofill will get in the way.

Reopening, adjusting the price. @situchan please review @ikevin127's proposal as a next step. 👍

[ikevin127]

@tienifr Yes, I can still reproduce the issue on Safari web, you need to have autocomplete login data saved in order to reproduce.

The mentioned FormElement component doesn't use html <form> (proposed solution), it's just a react native <View> wrapper if you check the component source.

If you cannot reproduce, let us now and we'll reassign somebody who can and wants to open the PR.

[tienifr]

@ikevin127 It's actually the form element when rendered to HTML thanks to role (you can check out the HTML source in devtools):

App/src/components/FormElement/index.tsx

Line 40 in e9c5e9b

role={ComponentUtils.ACCESSIBILITY_ROLE_FORM}

I'm trying again now.

[ikevin127]

@tienifr My bad I didn't notice the form role. Anyway, I can still reproduce the issue on staging and local dev without the proposed fix, just as shown in OP video.

I looked into why the parent <form> applied by FormElement doesn't fix the issue and the proposed solution does.
As a note, the solution was inspired by this SO comment.

The FormElement wrapper contains the following logic:

App/src/components/FormElement/index.tsx

Lines 18 to 36 in fb7fe9f

	useEffect(() => {
	const formCurrent = formRef.current;
	if (!formCurrent) {
	return;
	}
	// Prevent the browser from applying its own validation, which affects the email input
	formCurrent.setAttribute('novalidate', '');
	// Password Managers need these attributes to be able to identify the form elements properly.
	formCurrent.setAttribute('method', 'post');
	formCurrent.setAttribute('action', '/');
	formCurrent.addEventListener('submit', preventFormDefault);
	return () => {
	formCurrent.removeEventListener('submit', preventFormDefault);
	};
	}, []);

There's 2 ways in which, compared to the empty <form> proposed by the solution, the logic above facilitates the issue:

1. The presence of the novalidate attribute.
2. The lack of the dataset attribute, with empty object passed.

How did I test this ?
I commented out the novalidate attribute addition and added the dataset attribute like so:

        // Prevent the browser from applying its own validation, which affects the email input
+       // formCurrent.setAttribute('novalidate', '');
+       formCurrent.setAttribute('dataset', '{}');

and the result is that the issue is fixed because the form would behave like the proposed solution form wrapper.

The reason why I proposed wrapping the BaseValidateCodeForm with the empty <form> wrapper and only for Safari is because it does the following:

1. Maintains the FormElement logic / added attributes which are global (apply for all browsers).
2. Fixes our issue by wrapping only BaseValidateCodeForm which only contains the magic code / recovery code inputs.
3. It applies the fix for the issue targeted for Safari only.

Therefore the 2nd form wrapper will only show-up on Safari and will be an empty form with empty object dataset attribute applied like so:

<form dataset='[object Object]'>
...
</form>

which will fix our issue while not interfering with FormElement's logic.

Again

Please let me know ASAP if you CAN reproduce this and are willing to open the PR so we can move this forward right away as it has been dragging for a loooong time already.

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[MitchExpensify]

Reassigning while I'm out 🙇

[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 9.0.7-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: safari password autofill does not disappear #44888

If no regressions arise, payment will be issued on 2024-07-24. 🎊

For reference, here are some details about the assignees on this issue:

• @ikevin127 requires payment automatic offer (Contributor)
• @tienifr requires payment through NewDot Manual Requests

[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:

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127] 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:
• [@ikevin127] 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:
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.
• [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/414729

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Add conditional passwordless flow #13655
• [@ikevin127] 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: https://github.com/Expensify/App/pull/13655/files#r1685519848
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

Precondition: You have at least one login saved on Safari.

Note: The above autofill popup should only show-up on the Phone or email field.

1. Access the Expensify app on Safari web browser.
2. Select the the valid saved account using the autofil popup and click Continue.
3. Verify that on the next step -> the magic code screen, the email autofil popup is not visible.
4. Input the magic code and proceed with login.
5. Once logged-in, verify that the email autofill popup is not visible.

Do we agree 👍 or 👎.

[Christinadobrzyn]

Regression test - https://github.com/Expensify/Expensify/issues/414729

Payouts due:

• Contributor: $250 @ikevin127 (Paid in Upwork - https://www.upwork.com/nx/wm/offer/102946366)
• Contributor+: $250 @tienifr (payment through NewDot)

[Christinadobrzyn]

Payment day is here - payment summary - #32683 (comment)

Closing this as @tienifr will be paid through NewDot.