-
Notifications
You must be signed in to change notification settings - Fork 3k
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-24] [$500] mWeb - Scan - Page is skewed when dragging Scan content to the right #28689
Comments
Job added to Upwork: https://www.upwork.com/jobs/~0169e8ba6785ac7a07 |
Triggered auto assignment to @anmurali ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @narefyev91 ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.During copy text we can drag screen What is the root cause of that problem?On mobile devices What changes do you think we should make in order to solve the problem?The hard way For MoneyRequestSelectorPage We can create a new state that will be responsible for the selected text For selecting text we can use listener like
And when our state will not be empty
App/src/libs/Navigation/OnyxTabNavigator.js Lines 30 to 46 in 81a6a52
The easy way We can just turn off text selection App/src/pages/iou/ReceiptSelector/index.js Lines 152 to 166 in 81a6a52
What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.mWeb - Scan - Page is skewed when dragging Scan content to the right What is the root cause of that problem?The reason for this is with the way createMaterialTopTabNavigator is designed, when we pass multiple screens to createMaterialTopTabNavigator what it does is take these components and put them side by side like a carousel to allow for swiping, what this means is for the browsers this is just one element with bigger than screen width and so when selecting text, to the browser all text within the entire element is selectable together leading to the selection caret jumping to the next tab and causing misalignment What changes do you think we should make in order to solve the problem?We first should prevent the misalignment, we can do that by simply applying overflow clip to OnyxTabNavigator <OnyxTabNavigator
style={{ overflow: 'clip' }} Next we can create a manual handling of selection to not allow selection to go to the next tab, from what I could test it seem like the only tab this could happen is receiptSelector so we can make a function to handle the selection there: const onSelectionChange = () => {
const selection = window.getSelection();
const { anchorNode, anchorOffset, focusNode, focusOffset } = selection;
if (textRef.current) {
if (textRef.current.contains(focusNode)){
setSelectionPoints({
start: anchorOffset,
end: focusOffset
});
anchorNodeRef.current = anchorNode;
focusNodeRef.current = focusNode;
}
if (!textRef.current.contains(focusNode)) {
selection.removeAllRanges();
setTimeout(() => {
selection.setBaseAndExtent(anchorNodeRef.current, selectionPoints.start, focusNodeRef.current, selectionPoints.end);
}, 100);
}
}
}
useEffect(() => {
document.addEventListener('selectionchange', onSelectionChange);
return () => {
document.removeEventListener('selectionchange', onSelectionChange);
};
}, [selectionPoints]); By doing this we allow selection exclusivly to the element ref that we want to be selectable. ResultWhatsApp.Video.2023-10-04.at.5.52.06.PM.mp4 |
Proposal from @ZhenjaHorbach looks good to me #28689 (comment) |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@marcochavezf, @anmurali, @narefyev91 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
❌ There was an error making the offer to @ZhenjaHorbach for the Contributor role. The BZ member will need to manually hire the contributor. cc @thienlnam |
Thanks for the review @narefyev91, assigned @ZhenjaHorbach |
Not overdue, @ZhenjaHorbach when would a PR be ready for review? |
@marcochavezf |
Sure, I will try again |
🎯 ⚡️ Woah @narefyev91 / @ZhenjaHorbach, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.85-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-24. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
@marcochavezf, @anmurali, @narefyev91, @ZhenjaHorbach Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@marcochavezf, @anmurali, @narefyev91, @ZhenjaHorbach Still overdue 6 days?! Let's take care of this! |
Paid @ZhenjaHorbach with urgency bonus |
@narefyev91 can you complete the BZ checklist? |
|
I think there's nothing left here, sop I will close it out but feel free to re-open it if there's still something pending. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
The page will not be skewed.
Actual Result:
The page is skewed.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.76-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
Notes/Photos/Videos: Any additional supporting documentation
Bug6222685_1696298663769.Screen_Recording_20231003_034204_Chrome.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @anmuraliThe text was updated successfully, but these errors were encountered: