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 for payment 2023-10-27] [$500] Double tap on emoji selects am/pm of the time of the message #29021

Closed
2 of 6 tasks
m-natarajan opened this issue Oct 6, 2023 · 63 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Oct 6, 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!


Action Performed:

  1. Open the app
  2. Open any report
  3. Send any emoji or emoji plus text (ensure emoji is first)
  4. Double click on left half of emoji and observe that it select emoji plus AM/PM of above time

Expected Result:

App should not select AM/PM from time above on double click selection of emoji

Actual Result:

App selects AM/PM from time above on double click selection of emoji

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.79.-3
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
Notes/Photos/Videos: Any additional supporting documentation

mac.chrome.desktop.double.click.copy.AM.PM.mov
windows.chrome.double.tap.selects.AM.PM.mp4
Recording.10.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696582198838549

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0149fb66dc1d8f480d
  • Upwork Job ID: 1710346408992141312
  • Last Price Increase: 2023-10-06
  • Automatic offers:
    • dhanashree-sawant | Reporter | 27220198
Issue OwnerCurrent Issue Owner: @anmurali
@m-natarajan m-natarajan added the External Added to denote the issue can be worked on by a contributor label Oct 6, 2023
@melvin-bot melvin-bot bot changed the title Double tap on emoji selects am/pm of the time of the message [$500] Double tap on emoji selects am/pm of the time of the message Oct 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0149fb66dc1d8f480d

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

melvin-bot bot commented Oct 6, 2023

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Oct 6, 2023
@GianlucaMina
Copy link

GianlucaMina commented Oct 6, 2023

Proposal

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

Double tap on emoji selects am/pm of the time of the message

What is the root cause of that problem?

The problem occurs because in the rendering, am/pm and the emoji become the same word and when double-clicking both are selected as if they were one.

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

We must separate the words by adding a Non-breaking space at the end of the date (am/pm in the rendering) and add a style of userSelect: none so that the selection is clean.

<span style={{userSelect: "none"}}>&nbsp;</span>

The modification would occur on this line

return <Text style={[styles.chatItemMessageHeaderTimestamp]}>{props.datetimeToCalendarTime(props.created)}</Text>;

It would be something like this

return <Text style={[styles.chatItemMessageHeaderTimestamp]}>{props.datetimeToCalendarTime(props.created)}<span style={{userSelect: "none"}}>&nbsp;</span></Text>;

What alternative solutions did you explore? (Optional)

Alternatively the style can be moved to styles.js or another file of convenience

Final Result

4052c31f-61a2-415d-80ea-404717bbb1c5.webm
1adebe6f-45d1-4d96-aeec-3a7504007c39.mp4

@KunwarSingh
Copy link

KunwarSingh commented Oct 6, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.
Double tap on emoji selects am/pm of the time of the message

What is the root cause of that problem?

The problem is related to the way Chrome handles text selection in some cases. It can happen when there are overlapping elements or unusual styling applied to the content.

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

We must add a simple css style to am/pm div

What alternative solutions did you explore? (Optional)
NA

Contributor details

Your Expensify account email: thekunwarsingh@gmail.com
Upword Profile Link: https://www.upwork.com/freelancers/~0174c13534125c9866

@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

📣 @KunwarSingh! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@KunwarSingh
Copy link

Contributor details

Your Expensify account email: thekunwarsingh@gmail.com
Upword Profile Link: https://www.upwork.com/freelancers/~0174c13534125c9866

@KunwarSingh
Copy link

Proposal

Please re-state the problem that we are trying to solve in this issue.
Double tap on emoji selects am/pm of the time of the message

What is the root cause of that problem?

The problem is related to the way Chrome handles text selection in some cases. It can happen when there are overlapping elements or unusual styling applied to the content.

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

We must add a simple css style to am/pm div

What alternative solutions did you explore? (Optional)
NA

Contributor details

Your Expensify account email: thekunwarsingh@gmail.com
Upword Profile Link: https://www.upwork.com/freelancers/~0174c13534125c9866

@KunwarSingh
Copy link

KunwarSingh commented Oct 6, 2023

@narefyev91 I can quickly resolve this and push a fix to web and desktop app

fixed.mov

@ayazalavi
Copy link
Contributor

ayazalavi commented Oct 6, 2023

@KunwarSingh what is the css you are adding? Can you still select PM after adding css?

@KunwarSingh
Copy link

separately PM can be selected, but if we double click its not

@saranshbalyan-1234
Copy link
Contributor

Proposal

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

Upon selecting the emoji app selects AM/PM as well

What is the root cause of that problem?

The problem occurs because during rendering browser is unable to differentiate between text, it thinks it as one single word

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

<ReportActionItemDate created={props.action.created} />

We can store this style in styles.js, just for the proposal adding styles directly
We can return right after the above component <View style = {{userSelect:"none"}} >

       <ReportActionItemDate created={props.action.created} />
       <View style = {{userSelect:"none"}} ></View>

What alternative solutions did you explore? (Optional)

If we don't want to select AM/PM at all, we can directly pass userSelect:"none" in

App/src/styles/styles.js

Lines 1594 to 1600 in 73dcafd

chatItemMessageHeaderTimestamp: {
flexShrink: 0,
color: theme.textSupporting,
fontSize: variables.fontSizeSmall,
paddingTop: 2,
},

@ayazalavi
Copy link
Contributor

ayazalavi commented Oct 7, 2023

Proposal

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

In the web app and desktop app, in the chat listing, if the first chat of each group begins with an emoji as the first character, double-clicking it selects the last word of the previous line.

What is the root cause of that problem?

The root cause of this issue is the default behavior of Chromium, which selects emojis along with one extra word. This behavior is inherent in all browsers built using Chromium. Here are additional examples from other sites:

Screenshot 2023-10-06 at 11 30 58 PM Screenshot 2023-10-07 at 11 55 53 AM

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

We can avoid this issue by:

  1. Detecting if the first character of a particular chat is an emoji.
  2. Checking if it is the first chat item, as this issue only occurs when the emoji is the first character of the first chat item.
  3. Verifying that it's not a mobile browser since they all use WebKit.
  4. Using a Zero-width space character "&#x200b;​" and adding it within the component at the beginning of the main chat Text component.

To implement this, add the following code to EmojiUtils.js:

function firstLetterIsEmoji(message) {
    const trimmedMessage = Str.replaceAll(message.replace(/ /g, ''), '\n', '');
    const match = trimmedMessage.match(CONST.REGEX.EMOJIS);

    if (!match) {
        return false;
    }

    return trimmedMessage.indexOf(match[0]) === 0;    
}

Then, in

<ReportActionItemFragment
key={`actionFragment-${props.action.reportActionID}-${index}`}
fragment={fragment}
iouMessage={iouMessage}
add:

displayAsGroup={props.displayAsGroup}

Next at line 116, in

return <RenderHTML html={props.source === 'email' ? `<email-comment>${htmlWithTag}</email-comment>` : `<comment>${htmlWithTag}</comment>`} />;
}
const containsOnlyEmojis = EmojiUtils.containsOnlyEmojis(text);
add:

const firstLetterIsEmoji = EmojiUtils.firstLetterIsEmoji(text);

Now, before

add:

{firstLetterIsEmoji && !props.displayAsGroup && !isMobile() && <Text>&#x200b;</Text>}

The result can be seen here:

Untitled.mp4

What alternative solutions did you explore? (Optional)

The user-select: 'none' CSS attribute completely disables text selection within an HTML element. Using this CSS attribute requires caution. For instance, in the onPress event, you would need to disable the previous line's selection behavior and re-enable it when the user clicks anywhere else on the page, which can be a tricky process.

Another approach is to use window.getSelection and apply ranges to change selected text. However, this method is primarily suitable for web browsers, as the copy-paste functionality in native apps has a completely different implementation. Additionally, when working with mobile browsers, it's crucial to ensure that window.getSelection doesn't cause any issues, as support for the selection API is limited in mobile browsers.

@narefyev91
Copy link
Contributor

Proposal from @ayazalavi looks good to me #29021 (comment)
We should not use user-select: none for sure, also personally even adding "Zero-width space character" - looking like a hack to existing Chromium engine. We can try to use window.getSelection to identify which part of the elements we should select - and apply it only for web and desktop and ignore mobile fully.

But again not sure that we should fix browser normal behaviour. - will wait decision from assigned internal engineer to review
🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@ayazalavi
Copy link
Contributor

@narefyev91 When you select an emoji in your browser, it's not just one character; it's a combination of multiple characters, or "Unicode code points," that make up the emoji. The browser selects all of these code points to ensure the emoji is displayed correctly, which might appear as selecting one extra token causing issue mentioned in this ticket. We need to explicitly tell browser to stop collecting tokens in order to avoid this issue.

Zero-Width Characters: While this can technically work, it might not be the most intuitive solution and can make the code less readable for developers. It's generally better to use other methods if possible.

window.getSelection API: This is a more robust and standard way to manipulate text selection in the browser. You can use it to programmatically select and manipulate text without relying on special characters. This is a cleaner and more maintainable approach if you need to control text selection within your web application.

@GianlucaMina
Copy link

@narefyev91 Could you review my Proposal?, I think that using userSelect: none applied to a space is the best option since it solves the problem, while still allowing time selection without overcomplicating the problem and ensuring compatibility with all engines, not just chromium.

@m-natarajan m-natarajan added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 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 Overdue label Oct 11, 2023
@Li357
Copy link
Contributor

Li357 commented Oct 11, 2023

@narefyev91 I would say usually we wouldn't fix browser-normal behavior but I think we should fix this. As for proposals, I do think @ayazalavi has the better RCA, but it does seem quite complicated. Is there a downside to user-select: none on an empty element? It's not the most elegant but compared to adding a ton of checks to apply for the first character of the first message only on mobile isn't either.

@melvin-bot melvin-bot bot removed the Overdue label Oct 11, 2023
@GianlucaMina
Copy link

GianlucaMina commented Oct 11, 2023

@narefyev91 user-select: none has no problems other than readability, I proposed it in the simplest way in am/pm to avoid space problems in rendering, but it can also be transported to the text area (before the emoji) , and readability can be improved by sending it to its own component.

@sakluger sakluger changed the title [HOLD for payment 2023-10-30] [HOLD for payment 2023-10-27] [$500] Double tap on emoji selects am/pm of the time of the message [HOLD for payment 2023-10-27] [$500] Double tap on emoji selects am/pm of the time of the message Oct 27, 2023
@sakluger
Copy link
Contributor

@anmurali in case you missed it, this issue caused a regression (#30064) and a fix for that was deployed 1 week ago, so this is ready for payment now.

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 30, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@ayazalavi
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Oct 31, 2023
@ayazalavi
Copy link
Contributor

@anmurali still waiting for payments.

@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
Copy link

melvin-bot bot commented Nov 6, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Nov 8, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Nov 10, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@ayazalavi
Copy link
Contributor

ayazalavi commented Nov 10, 2023 via email

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 10, 2023
Copy link

melvin-bot bot commented Nov 13, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick!

2 similar comments
Copy link

melvin-bot bot commented Nov 13, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Nov 13, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@Li357
Copy link
Contributor

Li357 commented Nov 13, 2023

Any updates here?

Copy link

melvin-bot bot commented Nov 20, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

Copy link

melvin-bot bot commented Nov 22, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 10 days overdue. Is anyone even seeing these? Hello?

Copy link

melvin-bot bot commented Nov 24, 2023

@ayazalavi, @Li357, @anmurali, @narefyev91 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Nov 27, 2023
Copy link

melvin-bot bot commented Nov 27, 2023

This issue has not been updated in over 14 days. @ayazalavi, @Li357, @anmurali, @narefyev91 eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@melvin-bot melvin-bot bot added the Overdue label Dec 6, 2023
@anmurali
Copy link

anmurali commented Dec 6, 2023

@ayazalavi has been paid back on Nov 10. @narefyev91 is a call stack engineer and is handled via invoice. @dhanashree-sawant has also been paid. Closing!

@anmurali anmurali closed this as completed Dec 6, 2023
@melvin-bot melvin-bot bot removed the Overdue label Dec 6, 2023
@ayazalavi
Copy link
Contributor

@anmurali can you give me ratings in upwork please.
@narefyev91

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants