Closed izarutskaya closed 12 months ago
Triggered auto assignment to @bfitzexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are ✅)OK, let's get some proposals for this one.
Job added to Upwork: https://www.upwork.com/jobs/~01404d8d81934e3635
Triggered auto assignment to @muttmuure (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
when navigation is in progress if user long press the report mark as unread" popup opens inside report
The root cause is long press execution is not prevented if the navigation in progess resulting in opening of popover inside report.
In PR we are introducing a new hook `useSingleExecution` we can use the same in [OptionRowLHN](https://github.com/Expensify/App/blob/main/src/components/LHNOptionsList/OptionRowLHN.js)
```diff
+ const {isExecuting, singleExecution} = useSingleExecution();
+ const showPopover = (event) => {
+ if (isExecuting) {
+ return;
+ }
```
```diff
2 alternative solutions proposed - https://github.com/Expensify/App/issues/25460#issuecomment-1699553916
Where should I send initial proposal?
📣 @anukcr7! 📣 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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Prevent long press action to take place if user navigated to another screen.
The code that shows the pop up is here:
There is no debouce
, throttle
, or any sort of mutex lock in the function that prevents the pop up to be shown when the screen is already navigating away from LHN.
The event that occurs after a short press is a navigation to the Report page. Therefore we can use navigation to determine if the user has already short pressed the OptionRow, and if so prevent the pop up to appear despite the long press. This change would use useIsFocused
hook from react-navigation
or the withNavigationFocus
HOC already in the codebase.
Here is the change if using useIsFocused
:
On small screen devices, do not show popover if active route is the report screen .
// ...
import {Dimensions} from 'react-native'; // add this - using this instead of withDimensions prevents rerenders of list items when resizing screen
import Navigation from '../../libs/Navigation/Navigation'; // add this
import variables from '../../styles/variables'; // add this
function OptionRowLHN(props) {
const popoverAnchor = useRef(null);
const {translate} = useLocalize();
const showPopover = (event) => {
// add begin
const isSmallScreenWidth = Dimensions.get('window').width <= variables.mobileResponsiveWidthBreakpoint;
const isReportRoute = !!Navigation.getActiveRoute().match(CONST.REPORT_ROUTE_REGEX);
if (isSmallScreenWidth && isReportRoute) {
return;
}
// add end
setIsContextMenuActive(true);
ReportActionContextMenu.showContextMenu(
ContextMenuActions.CONTEXT_MENU_TYPES.REPORT,
// ...
~A slightly more complex change if the withNavigationFocus
HOC is used, including a prop rename since the OptionRowLHN already makes use of an isFocused
prop which is different from the usage in this proposal.~
~### However, I would like to know if the changes I am proposing would cause the entire list to be re-rendered when navigating since useIsFocused()
is updated when navigation occurs.~
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
Contributor details Your Expensify account email: annieemporium16@gmail.com Upwork Profile Link: (https://www.upwork.com/freelancers/anmolv3)
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
I have submitted the proposal on Upwork already
@anukcr7 and @Anmolverma19 information on how to submit proposals is in this document contributing guidelines.
Proposal
Please re-state the problem that we are trying to solve in this issue. Prevent long press action to take place if user navigated to another screen.
What is the root cause of that problem? According to me, Root cause lies in how touch and gesture events are handled and how UI/UX elements are hovered or triggered on certain user movement or usage
What changes do you think we should make in order to solve the problem? Here's what I initially plan to do to resolve it: To fix the issue where the "Mark as Unread" option appears when a user performs a long press on the left side of the screen while inside a report, I will need to make changes to the React Native codebase. Below are the steps to address this problem: Step 1: Locate the Code Responsible for the Long Press Gesture
Step 2: Identify the Event Handling Logic
Step 3: Modify or Remove the Event Handling Logic
Step 4: Test Extensively
Step 5: Document Changes
Step 6: Create a Pull Request
What alternative solutions did you explore? (Optional) Step 1: Identify the UI Element
Step 2: Apply CSS or Styling Changes
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
Does anyone here know how to run the web app on an android emulator? I'm getting the screen below.
Here is how it looks with my proposed solution.
https://github.com/Expensify/App/assets/25206487/9f4e9ed3-5a37-404c-8019-581a9a3c0c66
@carlosalmonte04 Only raise a PR once your proposal is selected by C+ reviewer.
we are implementing useSingleExecution
hook in this PR - https://github.com/Expensify/App/pull/24173, we can use the same in this case once PR is merged
Hello everyone. Thanks for your proposals. I would suggest everyone here to go through existing proposals in other issues (like: https://github.com/Expensify/App/issues/24420) to get idea of expected proposals.
We expect your proposal to include the root cause and link the permalink for the code where issue exists. Also, in your solution you are expected to link the code/file where you are suggesting the change and what sort of change are you suggesting.
Will wait for your updates.
Guideline cc: @carlosalmonte04 @Anmolverma19 @ishpaul777
The expected behaviour is unclear do we want to disregard the long press if navigation is in progress?
if yes, I guess this issue will be resolved when https://github.com/Expensify/App/pull/24173 is merged thats why i didn't added a proposal. In PR we are introducing a new hook useSingleExecution
which is used in PressableWithFeedback
, which might resolve the issue incase issue still persist we can use useSingleExecution
in OptionRowLHN
+ const {isExecuting, singleExecution} = useSingleExecution();
+ const showPopover = (event) => {
+ if (isExecuting) {
+ return;
+ }
<PressableWithSecondaryInteraction
ref={popoverAnchor}
onPress={(e) => {
if (e) {
e.preventDefault();
}
+ singleExecution(() => props.onSelectRow(optionItem, popoverAnchor))();
- props.onSelectRow(optionItem, popoverAnchor);
}}
cc- @sobitneupane
I tested the changes from https://github.com/Expensify/App/pull/24173 and it fixed this bug on my end.
tested #24173 on physical device this issue is still reproducable
https://github.com/Expensify/App/assets/104348397/d1200157-fae1-44d7-a70a-8978f547c99c
test with changes https://github.com/Expensify/App/issues/25460#issuecomment-1691339233 issue resolved -
https://github.com/Expensify/App/assets/104348397/561a9866-c051-4316-a4e6-769fe2349d9d
@sobitneupane Updated the comment to a formal propsal, let me know you thoughts on it once you get a chance.
I take my last comment back, I tested again and the changes from the useSingleExecution
PR do not fix this bug on its own. Also, the new useSingleExecution
hook changes the flag (isExecuting
) too fast. By the time the long press triggers the flag is already false
so it proceeds to show the pop up.
I think useSingleExecution
should accept an idleDuration
argument and change the flag (isExecuting
) to false
after the given idleDuration
.
Will review the proposals today.
any updates @sobitneupane ?
Thanks for the proposal everyone.
We have recently implemented a PR introducing a hook for single execution as mentioned by @ishpaul777. I believe we can make use of the hook here as well. Proposal from @ishpaul777 looks good to me.
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Hi @sobitneupane, I was testing my proposal on other platflorms and found it does not work as expected and issue still persisted. I came up with other solution to use useFocusEffect
hook - https://reactnavigation.org/docs/navigation-lifecycle/
implementation in OptionRowLHN -
const isNavigating = useRef(false)
useFocusEffect(
React.useCallback(() => {
isNavigating.current = false
return () => {
isNavigating.current = true
};
}, [])
);
const showPopover = (event) => {
if (isNavigating.current) {
return;
}
Result -
Hey @ishpaul777 that will trigger a re-render on every OptionRowLHN when navigation changes focus. I'd use Navigation.getActiveRoute
to compare routes if you must use navigation.
// ...
const showPopover = (event) => {
const route = Navigation.getActiveRoute();
if (route !== '/') {
return;
}
// ...
@carlosalmonte04 I don't think there's a rerender of optionRowLHN useFoucussedEffect sets isNavigating to true as soon as navigation starts (screen loose focus) and setIsNavigating to false when screen is focussed, can you link me to docs where it says it rerender component.
"The set function returned by useState lets you update the state to a different value and trigger a re-render.". Let me know if that helps clear it up, I could be missing something.
As far as i know, using setState dont have any performance regression in the case it rerenders the part of dom that is concerned with the state. i'll love to know @sobitneupane thoughts here. I have 2 alternative solutions here
const showPopover = (event) => {
if(isSmallScreenWidth && Navigation.getTopmostReportId()) {
return;
}
@sobitneupane @Gonals Can i get a review on updated changes
@Gonals @sobitneupane @bfitzexpensify @muttmuure 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!
@Gonals, @sobitneupane, @bfitzexpensify, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!
@ishpaul777 what were the issue you found with your proposal? In which platform and browser? Can you share the screen recording?
I cannot regularly reproduce the issue. Is there any way to reproduce the issue regularly?
Using useSingleExecution hook was not working at all. Issue is Easily reproduce able on mweb safari, high traffic account and a report that has a lot of messages.
It also tried using useWaitforNaviagation
implemented in the same PR as useSingleExecution
like this
singleExecution(() => (
waitForNavigate(() => {
props.onSelectRow(optionItem, popoverAnchor);
})()
))()
The issue still persists I might be missing something here? @sobitneupane
https://github.com/Expensify/App/assets/104348397/7bfac80d-3f0c-4510-a493-2ab75bd013ee
Thanks for the update @ishpaul777. I tested it and it didn't work for me as well. It is supposed to work for same user action. But we are dealing with different user actions (press and long press). So, it should be dealt uniquely.
@ishpaul777 Your proposal is similar to this propsoal.
I have other working solution too. https://github.com/Expensify/App/issues/25460#issuecomment-1699553916 @sobitneupane
const showPopover = (event) => {
if(isSmallScreenWidth && Navigation.getTopmostReportId()) {
return;
}
Gentle bump @sobitneupane
We might go with this proposal. I am yet to test it.
I tested the proposal from @redpanda-bit.
However, I would like to know if the changes I am proposing would cause the entire list to be re-rendered when navigating since useIsFocused() is updated when navigation occurs.
It turns out the above suspicion of re-rendering was correct and the entire list re-renders.
📣 @redpanda-bit! 📣 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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
~~@redpanda-bit You proposal implemented the useFocusEffect in a wrong way and doesn't solve the issue.
I updated my changes https://github.com/Expensify/App/issues/25460#issuecomment-1698131395, Indeed i used your idea to useRef for preventing rerender, but using useFocusEffect
was my idea in first place, hope you dont have any objection here?~~
Taking my words back your is correct also, but still i'd say useFocusEffect
was my idea in first place and your implementation is still hard to read.
useFocusEffect(
React.useCallback(() => {
// do something on focus screen so better and readable would be to set isFocusedRef/isNavigating to false
return () => {
// do something on blur screen so better and readable would be to set isFocusedRef/isNavigating to true
};
}, [])
);
@sobitneupane is there a proposal here we can move forward with?
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:
Expected Result:
App should not display mark as unread popup inside report
Actual Result:
On opening user report with lots of messages and quickly long press, mark as unread popup gets opened inside the report
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.55-4
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
https://github.com/Expensify/App/assets/115492554/ca751249-5f02-4bf5-bba9-c13376b48ef0
https://github.com/Expensify/App/assets/115492554/9fc1cea1-9fe0-4014-b605-2bac67ab14ce
Expensify/Expensify Issue URL:
Issue reported by: @Dhanashree-Sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691495713340339
View all open jobs on GitHub
Upwork Automation - Do Not Edit