[HOLD for payment 2024-02-14] [$500] Expense - Approver email from Next step message receives error when pasted in login field

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

Precondition:

• User is an employee of Collect workspace.
• The Collect workspace has admin, approver and employee.

1. [Employee] Create a manual request from the workspace chat.
2. [Admin] Go to workspace chat with Employee.
3. [Admin] Click on the expense preview to open expense report.
4. [Admin] Copy the approver email from Next step message.
5. [Admin] Paste the email in the login field.

Expected Result:

No validation error will be thrown as the email copied is a valid email.

Actual Result:

The email is a valid email but validation error shows up in the login field.

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

Bug6340063_1705059929889.bandicam_2024-01-11_10-09-45-385.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b45edbff007b5878
• Upwork Job ID: 1745790923128475648
• Last Price Increase: 2024-01-26
• Automatic offers:
• alitoshmatov | Reviewer | 28129669
• tienifr | Contributor | 28129670

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

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

Copying the email from next step message and use it as the login will fail the login validation.

What is the root cause of that problem?

The email in next step message contains a zero width character, specifically zero width space that is added from prefixMailSeparatorsWithBreakOpportunities.

App/src/libs/EmailUtils.ts

Lines 10 to 28 in 964548a

	/**
	* Prepends a zero-width space (U+200B) character before all `.` and `@` characters
	* in the email address to provide explicit line break opportunities for consistent
	* breaking across platforms.
	*
	* Note: as explained [here](https://github.com/Expensify/App/issues/30985#issuecomment-1815379835),
	* this only provides opportunities for line breaking (rather than forcing line breaks) that shall
	* be used by the platform implementation when there are no other customary rules applicable
	* and the text would otherwise overflow.
	* @param email - The email address to be sanitized
	* @returns The email with inserted line break opportunities
	*/
	function prefixMailSeparatorsWithBreakOpportunities(email: string) {
	return email.replace(
	/([.@])/g,
	// below: zero-width space (U+200B) character
	'​$1',
	);
	}

The motivation behind that is explained well in the comment.

So, when we use this as the login, the validation will throw an error because zero width scpae is not a valid character.

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

We can replace all zero width space when copying the text in SelectionScraper.

App/src/libs/SelectionScraper/index.ts

Lines 113 to 114 in 964548a

	if (dom.type.toString() === 'text' && dom instanceof DataNode) {
	data = Str.htmlEncode(dom.data);

data.replaceAll('\u200b', '')

Another zero width character is ZWJ with unicode of \u200d which maybe we can include too with any other zero width character.

or remove prefixMailSeparatorsWithBreakOpportunities if we don't want it anymore

[shubham1206agra]

Proposal

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

Expense - Approver email from Next step message receives error when pasted in login field.

What is the root cause of that problem?

The email in next step message contains a zero-width character, which is zero-width space character (so that user wouldn't notice this) that is added using prefixMailSeparatorsWithBreakOpportunities.

App/src/libs/EmailUtils.ts

Lines 10 to 28 in 964548a

	/**
	* Prepends a zero-width space (U+200B) character before all `.` and `@` characters
	* in the email address to provide explicit line break opportunities for consistent
	* breaking across platforms.
	*
	* Note: as explained [here](https://github.com/Expensify/App/issues/30985#issuecomment-1815379835),
	* this only provides opportunities for line breaking (rather than forcing line breaks) that shall
	* be used by the platform implementation when there are no other customary rules applicable
	* and the text would otherwise overflow.
	* @param email - The email address to be sanitized
	* @returns The email with inserted line break opportunities
	*/
	function prefixMailSeparatorsWithBreakOpportunities(email: string) {
	return email.replace(
	/([.@])/g,
	// below: zero-width space (U+200B) character
	'​$1',
	);
	}

As we are not removing zero-width characters in LoginForm as we are doing in every other form, which uses StringUtils.removeInvisibleCharacters in FormProvider definition of validation and submit methods.

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

Remove the zero-width characters using StringUtils.removeInvisibleCharacters in validateAndSubmitForm, and validate method in LoginForm.

[tienifr]

Proposal

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

The email is a valid email but validation error shows up in the login field.

What is the root cause of that problem?

In here, we're appending zero width character into the email so that it breaks properly on web, more explanation here.

This causes the validation to fail in login page.

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

I'm expanding on the suggested approach

1. Add testID="email-with-break-opportunities" to the Text component here to indicate what type of tag this is (we could name it next-step-email but I prefer email-with-break-opportunities because then we can reuse it wherever we want to target an email with break opportunities). This is an approach widely used in the app such as in here

2. In here, check if the parent of this text has the email-with-break-opportunities data-testid, if yes that means this is part (or whole) of an email that we prefixed with break opportunities and we know we should remove those added break opportunities from it when copying.

if (dom?.parent?.attribs?.[tagAttribute] === "email-with-break-opportunities") {
    data = data.replaceAll('\u200b', '');
}

(we can expand to remove all zero width characters if we want to)
(The attribs[tagAttribute] approach is widely used in SelectionScraper, like in here)

3. In other places where emails with break opportunities are used, also use the testID="email-with-break-opportunities" on the Text element, for example I can see another place here

4. We can optionally update so that we'll only do the zero width character replacement in web/desktop because those are the only places that we need the logic now (native platforms have no problems with breaking the email properly). That can be done by make prefixMailSeparatorsWithBreakOpportunities a no-op in native platforms.

We can additionally remove the invisible characters before validation/submission in the login page using StringUtils.removeInvisibleCharacters, so we don't accidentally show errors for emails with invisible characters copied from other sources.

What alternative solutions did you explore? (Optional)

I don't think this is good practice because:

• It can cause the copied text to fail randomly on other forms in the web (our form here is an example) because the zero width character is not common and most forms out there won't detect and remove it properly. And when it fails the user won't know why because it looks like any other email
• It causes weird things to happen. For example when you copied this text hello​@expensify.com which contains a zero width character before @, if you delete characters one by one when coming to the o before @, you'll have to delete twice for it to work (one delete is for the invisible character)
• We also shouldn't remove the zero width character when copying the text as suggested above because there might be valid use cases for it elsewhere in the app, like in messages

I think what we should do instead is to use word break opportunity tags (which is also mentioned as an alternative in this original explanation). The only drawback mentioned is that it only works for web. I think that's fine because all platforms like iOS and Android already have it built in, they break the lines properly, the issue is only for web. And this approach doesn't have all the drawbacks mentioned above.

More info on why the word break opportunity tags is better here https://medium.com/@igorshevchenko/wbr-is-better-than-zero-width-space-128f4bedc5e0#:~:text=According%20to%20the%20HTML5%20spec,intended%20for%20invisible%20word%20separation.
To quote:

But now I found that there’s issue which might make me switch back to using <wbr>: some smarty-pants IDE insert ZWSP “as is” when copying text from browser to, let’s say, search field. Nothing found. Disaster. Failure. Cataclysm.

[MitchExpensify]

Proposals ready for you to check out @alitoshmatov

[alitoshmatov]

@bernhardoj Thank you for proposal, your RCA is correct. I do agree with your solution, clearing zero width characters when copying is a good approach.

[alitoshmatov]

@shubham1206agra Thank you for proposal. Your solution does solve the issue but I am more worried about user copying zero-width characters without knowing them. And I agree with @tienifr 's point about it:

It can cause the copied text to fail randomly on other forms in the web (our form here is an example) because the zero width character is not common and most forms out there won't detect and remove it properly. And when it fails the user won't know why because it looks like any other email

[alitoshmatov]

@tienifr Thank you for your proposal and detailed explanation. I am more inclined to just remove zero-width characters when copying the text. I think this is the best solution since user will copy what they see.

We also shouldn't remove the zero width character when copying the text as suggested above because there might be valid use cases for it elsewhere in the app, like in messages

Can you give some detailed example for this point you made

[shubham1206agra]

@alitoshmatov The problem with wbr approach is that we don't know if it is implemented in react-native-render-html as we use the library to dynamically render html tags.

[alitoshmatov]

@shubham1206agra I agree, and I think we should avoid modifying that logic

[tienifr]

Can you give some detailed example for this point you made

@alitoshmatov It's usually used to mark word breaks in languages without visible space between words like Japanese, Thai. Also it can be used by engineering teams like ours which need to discuss usage of the invisible characters, amongst other use cases.

This may sound niche, but our aim is to be a very popular workspace messaging tool so I think we should be inclusive and not prevent users from such use case.

I tried Slack and WhatsApp and they also keep the text as is without modifying it like removing the zero width character.

[alitoshmatov]

@tienifr I see your point, I do agree that we shouldn't remove zero-width characters when user copies it. But in our case user is not aware that zero-width character exists in email and I think this is a problem.

[alitoshmatov]

I think this issue is very rare and it is not urgent. I think we benefit if we wait for some more proposals.

[bernhardoj]

This may sound niche, but our aim is to be a very popular workspace messaging tool so I think we should be inclusive and not prevent users from such use case.

Real users won't bother to put zero-width space to their message for each Thai/Japanese word. Even if someone does, when they copy a Thai/Japanese message, will they expect the message already contain zero-width space between words? I think no.

I tried Slack and it even clears immediately the zero-width space when pasting.

Screen.Recording.2024-01-17.at.15.30.19.mov

(notice the send button blinking)

[tienifr]

Real users won't bother to put zero-width space to their message for each Thai/Japanese word. Even if someone does, when they copy a Thai/Japanese message

@bernhardoj they don't "put" it, it might just be how their language works and their language-based applications work.

I tried Slack and it even clears immediately the zero-width space when pasting.

@bernhardoj please try the zero-width space between words, like this one hello​there, then send it, then copy it and paste to another system, you'll see they retain the zero-width spaces.

[tienifr]

@alitoshmatov how about we always render the email that has prefixMailSeparatorsWithBreakOpportunities inside a html tag (or a wrapper with a custom html attribute), then when we copy it we only remove the zero width space of the text that are inside such html tag (or a wrapper with a custom html attribute). It's like we have something to distinguish that "this is the text that we ourselves append zero-width spaces to" and apply the ZWS removing logic only for that text.

As far as I can see we currently have that with emails in a few places so that can work.

Do you think it's better?

[bernhardoj]

please try the zero-width space between words, like this one hello​there

Ok, this one works

they don't "put" it, it might just be how their language works and their language-based applications work.

I tried copying several Japanese articles and none of them contain zero width space.

[alitoshmatov]

@tienifr That sounds like viable option. Let me do some research

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[MitchExpensify]

@alitoshmatov how do @tienifr 's updates look to you?

[alitoshmatov]

Nice! @tienifr 's proposal looks great.

We can additionally remove the invisible characters before validation/submission in the login page

I think our goal here should if we changed A to B we should also reverse it to A at the end. Not less not more.

I think we can start working on PR if internal engineer approves our approach.
C+ reviewed 🎀 👀 🎀

[melvin-bot]

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

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

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

Offer link
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]

PR ready for review #35557.

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

• Remove zero width space when copying email #35557

If no regressions arise, payment will be issued on 2024-02-14. 🎊

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

• @alitoshmatov requires payment automatic offer (Reviewer)
• @tienifr requires payment automatic offer (Contributor)

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

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

[MitchExpensify]

Paid and contracts ended! Bump on the BZs steps @alitoshmatov 🙇

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/31116/files
• 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/31116/files#r1492974294
• 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
• Determine if we should create a regression test for this bug. No need

[MitchExpensify]

Thanks @alitoshmatov