Closed luacmartins closed 3 months ago
Triggered auto assignment to @dylanexpensify (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.
Job added to Upwork: https://www.upwork.com/jobs/~013a5b2e0e021c1c12
Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External
)
Upwork job price has been updated to $125
Contributor details Your Expensify account email: neupanebishal07@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01182d696a155c95ce
β Contributor details stored successfully. Thank you for contributing to Expensify!
Row is not highlighted
When we hover on a Pressable
nested inside another Pressable
, by default it will trigger onHoverOut
of the outer Pressable
. This is a react-native-web
feature when the contain
here is true
, so when another Pressable
is hovered, the current hovered Pressable will automatically lose hover state, even though the newly hovered element is inside the previously hovered element, and the browser doesn't dispatch any pointerleave
event on the previously hovered element. From the POV the parent component is still hovered, but react-native-web
still triggers the onHoverOut
for the above reason.
In https://github.com/Expensify/App/blob/fb562ec159d575a1d71dd1a3d0c9f6551cf6a3f5/src/components/Pressable/PressableWithFeedback.tsx#L69, if event.type
is pointerenter
or mouseenter
, that means the hover is lost due to user entering a Pressable inside the current component, early return because the hover is still inside the current component. Alternatively we can add a prop to control this behavior like allowNestedHover
.
if (event?.type === 'pointerenter' || event?.type === 'mouseenter') {
return;
}
Then isHovered
needs to be passed as prop to GenericPressable
and in this, hoverStyle
will also be applied if isHovered
is true
(There're other ways to check like: check the target of the event
is inside the current component)
Optionally, we can do the same in https://github.com/Expensify/App/blob/fb562ec159d575a1d71dd1a3d0c9f6551cf6a3f5/src/components/Pressable/PressableWithFeedback.tsx#L62, if isHovered
is already true
, early return
Add a PR upstream in react-native-web
so we can control the contain
here via a prop. Then we can set it to false
by using the prop of Pressable
. If it's false
, hovering on the nested button will still keep the hover on the parent button.
Updated proposal to add an alternative solution
Child hover effect not detected by parent Pressable
By default, we don't have a system or a feature that let hovered child Pressable, keep the hover style of a parent Pressable
On this PR https://github.com/Expensify/App/pull/42823 I created a mouseContext to help elimanate the issue between parent and child when there's a mouseup/moueUp/mouseDown event.
We can use the same context and add a new feature for child hover state
import type { ReactNode } from 'react';
import React, { createContext, useContext, useMemo, useState } from 'react';
type MouseContextProps = {
isMouseDownOnInput: boolean;
setMouseDown: () => void;
setMouseUp: () => void;
isChildHovered: boolean;
setChildHover: () => void;
setChildLeave: () => void;
};
const MouseContext = createContext<MouseContextProps>({
isMouseDownOnInput: false,
setMouseDown: () => {},
setMouseUp: () => {},
isChildHovered: false,
setChildHover: () => {},
setChildLeave: () => {},
});
type MouseProviderProps = {
children: ReactNode;
};
function MouseProvider({ children }: MouseProviderProps) {
const [isMouseDownOnInput, setIsMouseDownOnInput] = useState(false);
const [isChildHovered, setIsChildHovered] = useState(false);
const setMouseDown = () => setIsMouseDownOnInput(true);
const setMouseUp = () => setIsMouseDownOnInput(false);
const setChildHover = () => setIsChildHovered(true);
const setChildLeave = () => setIsChildHovered(false);
const value = useMemo(
() => ({
isMouseDownOnInput,
setMouseDown,
setMouseUp,
isChildHovered,
setChildHover,
setChildLeave,
}),
[isMouseDownOnInput, isChildHovered]
);
return <MouseContext.Provider value={value}>{children}</MouseContext.Provider>;
}
const useMouseContext = () => useContext(MouseContext);
export { MouseProvider, useMouseContext };
Then with this mouse context, we can fix all the BaseListItem
components, where the effect will be also applied to other UIs like (where we have the same issue)
Using the new context, we can use BaseListItem
component, then:
onMouseEnter={setChildHover}
onMouseLeave={setChildLeave}
@alitoshmatov can you review these proposals, please? Thanks!
@luacmartins, @alitoshmatov, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
bump @alitoshmatov
@luacmartins, @alitoshmatov, @dylanexpensify Huh... This is 4 days overdue. Who can take care of this?
@luacmartins Should this logic applied everywhere - Parent hover should be active when cursor is on pressable element
We have similar case Search page:
https://github.com/Expensify/App/assets/59907218/49f5897d-1b1b-4bfa-b63b-1882a2c92c28
Workspace list:
https://github.com/Expensify/App/assets/59907218/02d7c1a3-97db-4b92-b495-4bd089ce4626
@dominictb Thank you for your proposal, I really like the your first solution but it is not working for me can you recheck it?
Applying early return:
if (event?.type === 'pointerenter' || event?.type === 'mouseenter') {
return;
}
@dragnoir Thank you for your proposal, I think your solution is not directly solving the problem we should change the behavior of hover element when it contains pressable element. I think your solution is adding a new behavior to each element manually.
@Expensify/design would love your thoughts on https://github.com/Expensify/App/issues/43233#issuecomment-2165330813
We definitely don't want the behavior in the link above.
Goal: interacting with a child element of a parent, where the parent has :hoverable styles, should not interrupt the hover style of the parent. Does that make sense?
Ok, so seems like we should update all those cases as part of this issue as well
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
Yeah, that makes sense to me!
Agree we should update all the cases as part of this.
it is not working for me can you recheck it?
@alitoshmatov It works fine for me, did you also include this change? (also there in the proposal)
Then isHovered needs to be passed as prop to GenericPressable and in this, hoverStyle will also be applied if isHovered is true
Agreed, @alitoshmatov mind giving us an update here?
@dominictb Thanks, I did manage to apply your changes and it works most of the time. It is not working in workspace list(second video). We need this to work in every instances.
It looks like this is not the first issue about this exact problem, https://github.com/necolas/react-native-web/issues/1875 have been mention in couple of expensify issues.
I think we should take this also into consideration and solve this issue so that it won't break any existing behavior
It is not working in workspace list(https://github.com/Expensify/App/issues/43233#issuecomment-2165330813)
@alitoshmatov It's the case because workspace list is using PressableWithoutFeedback
To make it work for all cases we can just use the same solution in GenericPressable
rather than PressableWithFeedback
, then it will work for any pressable in the app.
That means:
const [isHovered, setIsHovered] = useState(false);
onHoverIn
, onHoverOut
event to update the state after this line
onHoverOut={(event) => {
if (event?.type === 'pointerenter' || event?.type === 'mouseenter') {
return;
}
setIsHovered(false);
if (rest.onHoverOut) {
rest.onHoverOut(event);
}
}}
onHoverIn={(event) => {
setIsHovered(true);
if (rest.onHoverIn) {
rest.onHoverIn(event);
}
}}
(state.hovered || isHovered) && StyleUtils.parseStyleFromFunction(hoverStyle, state),
hovered
state is correct
{(state) => (typeof children === 'function' ? children({...state, isScreenReaderActive, isDisabled, hovered: state.hovered || isHovered}) : children)}
That looks good, I think we can go with @dominictb 's solution here
C+ reviewed π π π
Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
π£ @alitoshmatov π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @dominictb π 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 π
@luacmartins @alitoshmatov @dylanexpensify @dominictb 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!
PR will be ready soon!
Nice! Let's get it!
Waiting for regression period!
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:
For reference, here are some details about the assignees on this 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:
Regression Test Proposal
View
button inside the rowDo we agree π or π
payment coming up!
Payment summary:
Please apply/request!
@dominictb please accept offer! πββοΈ
@luacmartins @dylanexpensify Could we keep the $250 original price for this issue?
I saw it being reduced by half, however I believe this is not a simple issue, it requires non-intuitive use of mouse event, and I needed to dig into the library being used to find the RCA. Also there was a request to fix all cases of Pressables across the app and we've done that, it requires a lot of testing to make sure there's no regression.
Appreciate your thought on this one!
Sure, we can keep this one at the regular bounty.
Upwork job price has been updated to $250
Nice, @dominictb accept the offer as it is, and I'll add another $125 as a bonus! @alitoshmatov I'll send a bonus for same amount to you as well!
Thank you @luacmartins @dylanexpensify
I accepted the offer
Nice, paying!
all done!
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: Reproducible in staging?: Reproducible in production?: 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: Slack conversation:
Action Performed:
Coming from this comment
Expected Result:
Row should be highlighted
Actual Result:
Row is not highlighted
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?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dylanexpensify