[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Settings - When navigated to wallet page, blue bubble selection option is shown.

[izarutskaya]

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.4.67-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Tap settings
3. Long press and select the email id displayed under avatar
4. Tap Wallet

Expected Result:

When navigated to wallet page, blue bubble selection option should not be shown.

Actual Result:

When navigated to wallet page, blue bubble selection option is shown.

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

Bug6464872_1714375428526.cb.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01626cae18fdbd316c
• Upwork Job ID: 1785760528249442304
• Last Price Increase: 2024-05-29

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[JmillsExpensify]

Adding this to #vip-split and opening up to the community.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01626cae18fdbd316c

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify, @sobitneupane Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

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

[melvin-bot]

@JmillsExpensify, @sobitneupane 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[sobitneupane]

Waiting Proposal

[melvin-bot]

@JmillsExpensify @sobitneupane this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@JmillsExpensify, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[JmillsExpensify]

Still waiting for proposals

[melvin-bot]

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

[dominictb]

Proposal

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

When navigated to wallet page, blue bubble selection option is shown.

What is the root cause of that problem?

I believe that when we push new route in the navigation stack, the old page (with all of the components) still exist in the document tree. We can see the in the Chrome DevTools

Hence we can still see the selection even if we move to another screen like in the issue description. If the navigation some how remove the HTML tags from the document tree, we won't see the selection state again.

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

OLD SOLUTION WHICH CAUSES REGRESSION

If the navigation state change, we should clear out the selection in the window. This can be done in the NavigationRoot component handleStateChange function

const handleStateChange = (state: NavigationState | undefined) => {
        ..... // other stuff

        // We want to clear the selection in the window
        window.getSelection()?.removeAllRanges();
    };

[NEW SOLUTION]

Draft PR: #43456

If the navigation state change, we should clear out the selection in the window. This can be done in the NavigationRoot component handleStateChange function

const handleStateChange = (state: NavigationState | undefined) => {
        ..... // other stuff

        // We want to clear the selection in the window
        clearRedundantSelectionRange();
    };

The point is clearRedundantSelectionRanges cannot be naive as window.getSelection().removeAllRanges() as it will causing this regression #43375. In order to avoid this we'll need to loop through all range in the selection (in case of Chrome, we only have 1 range):

const selection = window.getSelection();
  
if(!selection || !selection.rangeCount) {
    return;
  }

  let rangesNeedToBeRemove: Range[] = [];
  for(let i = 0 ; i < selection.rangeCount; ++i) {
    const range = selection.getRangeAt(i);
    if(!shouldThisRangeBeRemoved(range)) {
      continue;
    }
    rangesNeedToBeRemove.push(range);
  }

  rangesNeedToBeRemove.forEach((range) => {
    selection.removeRange(range);
  });

Since the original issue is related to a feature in Chrome called tap-to-search, and based on this article https://developer.chrome.com/blog/tap-to-search, we can argue that as long as the selection range doesn't belong to any editable component, it is safe to remove them.

To check that, we'll need find the closest ancestor of the range.commonAncestorContainer which is an element node (since the selection range container can be text node). If we couldn't find it, we can assume that this range is not editable, i.e return true (safe to remove). We can even add an additional check to check if document.designMode === 'on'

In case we could find that node element ancestor, we should check either the node is input or textarea or node.isContentEditable = true. If this is the case, the the range shouldn't be removed, or else we are safe to remove it.

shouldThisRangeBeRemoved(range) {
 const node = range.commonAncestorContainer as HTMLElement
 // w
  let ancestorNodeToCheck = node.nodeType === Node.ELEMENT_NODE ? node : node.parentElement;
  while(ancestorNodeToCheck && ancestorNodeToCheck.nodeType !== Node.ELEMENT_NODE) {
    ancestorNodeToCheck = ancestorNodeToCheck.parentElement;
  }
  
   // couldn't find closest ancestor node that is an element node
    if(!ancestorNodeToCheck) {
      return document.designMode.toLowerCase() !== 'on'
    }
  
    const nodeName = ancestorNodeToCheck.nodeName.toLowerCase();
  
    if(nodeName === 'input' || nodeName === 'textarea' || ancestorNodeToCheck.isContentEditable) {
      return false;
  }
}

Result:

android-8MB.mp4

In case the selection range is from the Composor, it won't be cleared

aw8.mp4

What alternative solutions did you explore? (Optional)

N/A

[sobitneupane]

Thanks for the proposal @dominictb

It looks like you're new to Expensify. Welcome!

New contributors are limited to working on one job at a time, however experienced contributors may work on numerous jobs simultaneously.

As per the contributing guidelines, new contributor can only work on one issue at a time.

Please let me know if you are assigned to a closed issue or an open issue with the Awaiting payment label.

[melvin-bot]

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

• chore: remove all selection ranges during navigation #42961

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

• @sobitneupane requires payment through NewDot Manual Requests
• @dominictb requires payment (Needs manual offer from BZ)

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

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

[dominictb]

the risks outweigh the rewards given the benefit of hindsight

It would be great if you guys can explain this.

I believe that the risks have already been outweighs the rewards from the beginning, it's just that my initial solution is so naive/simple that I(or we) overlook the risks. But in the second solution, I had fine-tuned the mechanism in the first solution, but not merely based on the regression bug we had, but with careful consideration of other factors to cover all of the scenarios (as I have explained above). What I'm trying to say is that the second solution is not a reaction to the recent regression, but a well-calibrated one which I (somehow) overlooked when proposing the initial solution, so it shouldn't counted as an temporary workaround for the regression, but a (probably) complete solution to this issue without causing any further regression. With that in mind, I believe that it's worth pursuing the second solution as the calculated risks have been lowered.

I'm happy with any final decision, but it would be nice to understand thoroughly why we shouldn't go forward with the new solution I proposed.

[amyevans]

I'm unassigning since I'm headed out on extended leave after today. If you need input from another internal engineer feel free to do the 🎀 trick! 👋

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @sobitneupane owed $125 via NewDot
• Contributor: @dominictb paid $125 via Upwork

BugZero Checklist (@JmillsExpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1785760528249442304/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[JmillsExpensify]

I agree with Amy. I've updated the payment summary above to reflect a payout of $125.

[JmillsExpensify]

@dominictb mind sharing your Upwork profile so that I can invite you to the Upwork job? The previous one expired.

[dominictb]

@JmillsExpensify Sure, here's my profile link on Upwork: https://www.upwork.com/freelancers/~01f70bed1934fd35d5

[JmillsExpensify]

Offer sent

[JmillsExpensify]

All paid out! @sobitneupane please complete the checklist and request payments in NewDot.

[JmillsExpensify]

Whoops @dominictb I'm still waiting for you to accept the offer in Upwork.

[dominictb]

@JmillsExpensify I accepted it, TY

[JmillsExpensify]

All paid out!

[trjExpensify]

Confirming payment summary as follows:

• $125 to @sobitneupane for the C+ review
• $125 to @dominictb for the fix

[JmillsExpensify]

$125 approved for @sobitneupane