[$500] Task - Hand cursor is displayed when hovering over the task description placeholder

[lanitochka17]

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: 1.4.32-2
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open a chat
2. Go to the assign task
3. Hover over the description

Expected Result:

Hand cursor should not be displayed when hovering over the task description placeholder

Actual Result:

Hand cursor is displayed when hovering over the task description placeholder

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

Add any screenshot/video evidence

Bug6355223_1706222101562.2024-01-26_03-31-46.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bb349ec7e874f74f
• Upwork Job ID: 1750652923015548928
• Last Price Increase: 2024-02-01
• Automatic offers:
• ikevin127 | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01bb349ec7e874f74f

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

Proposal

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

Hand cursor is displayed when hovering over the task description placeholder.

What is the root cause of that problem?

App/src/components/TextInput/BaseTextInput/index.tsx

Line 320 in ac94dc5

<View style={[styles.textInputAndIconContainer, isMultiline && hasLabel && styles.textInputMultilineContainer, styles.pointerEventsBoxNone]}>

We're using cursor: pointer on the task description label.

But due to the fact that the input container has styles.textInputMultilineContainer which is paddingTop: 23, and the container inherits the GenericPressable's cursor.pointer styles because:

App/src/components/Pressable/GenericPressable/BaseGenericPressable.tsx

Line 74 in ac94dc5

return styles.cursorPointer;

on this line there's no accesibilityRole or role passed down to the GenericPressable therefore we get the cursor.pointer and we see the pointer cursor.

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

We should change the style to cursor: default.

To do this, within BaseTextInput we have to pass down the role prop to the PressableWithoutFeedback. This will make it such that the inherited cursor will be default instead of pointer.

What alternative solutions did you explore? (Optional)

We can also do this by adding styles.cursorDefault within BaseTextInput's wrapper where the paddingTop: 23 is passed.

Videos

MacOS: Chrome / Safari

Before	After
Screen.Recording.2024-01-26.at.01.29.22.mov	Screen.Recording.2024-01-26.at.01.30.54.mov

[Krishna2323]

Proposal

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

Task - Hand cursor is displayed when hovering over the task description placeholder

What is the root cause of that problem?

We are applying padding top of 23px to multiline input containers, thats why the cursor text is not applied because the text input is 23px below from the container.

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

We should also apply cursor:'text'/styles.cursorText when we apply padding top of 23px. We need to update textInputMultilineContainer to also include cursor:'text'/styles.cursorText.

App/src/styles/index.ts

Line 1024 in ac94dc5

textInputMultilineContainer: {

Note: I believe that there is no difference in applying cursorText or padding to pressable or parent view or to the textInputMultilineContainer, all works the same way, the crucial part here is the root cause & my proposal was the first to provide correct root cause & solution.

Result

demo_cursor_text.mp4

[Krishna2323]

@ikevin127, pls comment proposal updated if you are updating to similar root cause or solution to other proposals, just to avoid confusions.

cc: @rushatgabhane

[Krishna2323]

Proposal Update

Included screenshots.

[Krishna2323]

Proposal Update

Added a note.

[tienifr]

Proposal

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

Hand cursor is displayed when hovering over the task description placeholder

What is the root cause of that problem?

We has paddingTop: 23px in container

App/src/components/TextInput/BaseTextInput/index.tsx

Line 320 in f6d2f61

<View style={[styles.textInputAndIconContainer, isMultiline && hasLabel && styles.textInputMultilineContainer, styles.pointerEventsBoxNone]}>

to make the text area visible when we have label

Unfortunately, the container inherits the GenericPressable's cursor.pointer styles (cursor: pointer) that why when we hover on the label, the cursor pointer is shown

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

Why does this problem happen on title?
Because we apply the paddingTop for input.

We should apply the padding top for the textarea as well for consistency

Move paddingTop

isMultiline && hasLabel && {paddingTop: 23},

to below this line

App/src/components/TextInput/BaseTextInput/index.tsx

Line 353 in f6d2f61

(!hasLabel || isMultiline) && styles.pv0,

We can define the variable for {paddingTop: 23} in styles.ts

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2024-01-26.at.11.10.40.mov

[dragnoir]

Proposal

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

Wrong cursor displayed hovering description top area.

What is the root cause of that problem?

To understand what's happening, let's add a green background to the View wrapping the <RNTextInput> component.

As on the screenshot, there's another layer at the top. This is because styles.textInputMultilineContainer is adding an extra padding.

When this padding is added, the extra zone (padding/light green on screenshot above) will ask about the style of the cursor state on hover.
De we deliver it? NO, then this extra zone will check parents for the cursor state and because the 1st pârent with the state set is PressableWithoutFeedback then the cursos will be pointer

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

Where we set styles.textInputMultilineContainer , we need to make sure of the cursos state on hover with styles.cursorText

<View style={[
  styles.textInputAndIconContainer, 
  isMultiline && hasLabel && styles.textInputMultilineContainer, 
  styles.pointerEventsBoxNone
+ styles.cursorText
]}>

POC:

2024-01-26.16.54.42.mp4

[melvin-bot]

@abekkala, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abekkala]

@rushatgabhane do you have any feedback for the proposals above?

[rushatgabhane]

reviewing

[ikevin127]

Just wanted to mention my proposal vs the second one's timestamp snapshots to avoid any confusions as my colleague mentioned in the beginning.

In chronological order:

ikevin127: Jan 26, 2024, 12:57 AM GMT+2 - initial proposal timestamp
Krishna2323: Jan 26, 2024, 1:01 AM GMT+2 - initial proposal timestamp
Krishna2323: between 1:01 and 1:22 - had 4 proposal edits, his last one at 1:22 looking like this:

Krishna2323's 4th edit @ 1:22

ikevin127: Jan 26, 2024, 1:28 AM GMT+2 - 1st edit of proposal timestamp - added more context to the initial proposal, being first to mention the full RCA (why do we see the pointer cursor and where is the style coming from)
ikevin127: Jan 26, 2024, 1:33 AM GMT+2 - 2nd edit of proposal timestamp - I added video of the solution result

[Krishna2323]

@ikevin127, your first edit( which has the same root cause and solution as mine initial proposal) came 27mins after my initial proposal.

This is your initial proposal:

This is my initial proposal, which has the root cause & solution:

[ikevin127]

Correction: your initial proposal had and still has the incomplete RCA 🙂

[Krishna2323]

This is was my latest proposal which was still before your first edit, it has the RCA which can be easily understood by anyone.

and the container inherits the GenericPressable's cursor.pointer styles because

The issue lies in why the text cursor is not applied, not why the hand cursor is present. The text cursor will inherit any styles from the parent, even if there is no pressable container, it would inherit default cursor from div.

[ikevin127]

Incorrect, if you take a closer look at the issue's title, actual result and expected result - one can clearly deduce that when the proposal's template What is the root cause of that problem? question is asked, the expected RCA here would be to have a clear understanding of why we see the hand (pointer) cursor.

Regarding your argument, I think that only mentioning the padding does not fulfil this analysis IMO as that's just the reason that causes the hand cursor to be seen, without any mention of where is that styling coming from. This way you'll apply a fix to something you don't fully understand.

We all know that without a clear understanding of the root cause, a solution cannot be applied - no matter whether or not it looks good or it fixes the issue (apparently). See example: #35197 (comment)

[Krishna2323]

@ikevin127, I'm sorry to say, but your arguments always seem redundant. How can you compare that issue with such a minor styling fix? You clearly copied my proposal and are now trying to prove my solution wrong.

let @rushatgabhane decide, I don't want to argue more.

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

@abekkala @rushatgabhane @aldo-expensify 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!

[abekkala]

@aldo-expensify can you review this please?

Looks like there may be some confusion on the proposal selected by @rushatgabhane

[aldo-expensify]

@ikevin127 i don't think we should change default cursor type of BaseGenericPressable. Because a Pressable is meant to be clicked and should have the hand cursor

I see your point here, but in this case it doesn't make sense to me to try to preserve the pointer cursor of the "pressable" if we really don't want to show it when it wraps an input. Also, I would assume BaseGenericPressable has the role prop just for cases like this.

Playing with the padding so it becomes impossible to hover just over the "pressable" feels more brittle to me than just aligning the role of the "pressable" with the input that is wrapping.

I prefer @ikevin127 's proposal that adds role={inputProps.role} because it is simple and more robust in my opinion

@Krishna2323 's proposal is also simple, but I like it better the use of role to solve the issue.

@ikevin127 won by timestamp on posting a proposal first, but that first proposal is veeery generic:

• The RCA is just guess anyone could take based on the pointer cursor appearing, and
• proposed fix is very generic just like the RCA

In summary, the way I see things is:

• @ikevin127 posted first a very generic proposal with little specific details that I would have rejected
• @Krishna2323 posted a proposal with a RCA that shows to me that an investigation did happen. I would consider this the first real proposal
• @ikevin127 and @Krishna2323 continued to polish their proposals (I assume that @ikevin127 used @Krishna2323 's findings)
• I consider the current proposals RCAs the same and consider that @Krishna2323 found it first
• I'm see @ikevin127 's solution different enough from @Krishna2323 's solution to consider it as a different proposal
• I like more @ikevin127 's role solution

[aldo-expensify]

About other proposals:

• @tienifr 's proposed solution based on adjusting padding, which is something I would prefer to avoid
• @dragnoir 's is a bit different, but I still prefer fixing this using role. I do appreciate though the images to show the padding issue, they are helpful to visualize the problem!

[Krishna2323]

@aldo-expensify thanks for the review, I still don't think adding role is different from adding styles on view, in both cases we are setting the parent's container style to cursorText and that's not very major point to differentiate a proposal, @ikevin127 has literally nothing in his proposal until I commented.

I'm sorry for the argument but I think it's unfair to assign @ikevin127 just because of a minor point. Thanks 🥲

[aldo-expensify]

Oops, I thought your proposal was to adjust paddings hehe, I see now that you are proposing change the cursor style. Re-reading again..................

[ikevin127]

I think @aldo-expensify's assessment is fair here. I was amazed that @rushatgabhane did not go with the role based solution once I explained that we have that prop on Pressables for a reason.

My RCA identified the full root cause (why we see the hand cursor, where is the style coming from) and came up with the correct solution which is to use the Pressable's role which will return text cursor if passd, instead of adding that style directly where the other proposal suggested - this would mean that the Pressable will still have the pointer cursor, we would just override it from a child of the Pressable.

[Krishna2323]

@ikevin127, your proposal was not selected because you copied, sorry for being rude but you are trying to put redundant arguments to create confusions, does applying role wouldn't override the cursor style of the parent component? Does role wouldn't override pressable's default cursor style? Is there any major difference in applying role to parent pressable or view?

Also can you look at your initial proposal?

cc: @aldo-expensify

[ikevin127]

does applying role wouldn't override the cursor style of the parent component? Does role wouldn't override pressable's default cursor style?

Nope, this is the point I'm trying to make here - since the Pressable is the parent that wraps the input but we're not passing the role of the input to the Pressable, its function here will return the pointer style cursor:

App/src/components/Pressable/GenericPressable/BaseGenericPressable.tsx

Lines 64 to 75 in 43d1576

	/**
	* Returns the cursor style based on the state of Pressable
	*/
	const cursorStyle = useMemo(() => {
	if (shouldUseDisabledCursor) {
	return styles.cursorDisabled;
	}
	if ([rest.accessibilityRole, rest.role].includes(CONST.ROLE.PRESENTATION)) {
	return styles.cursorText;
	}
	return styles.cursorPointer;
	}, [styles, shouldUseDisabledCursor, rest.accessibilityRole, rest.role]);

Passing the input role to the Pressable would simply return the correct style which is cursor text in this case.

I don't understand how can somebody call this overriding ?
Overriding would be to set the styles as the other proposal suggested, directly to the children of the Pressable, while the Pressable still returns cursor pointer because the role wasn't passed.

No matter how much you throw accusations, call my clarifications redundant and that I'm trying to create confusion - this is still the reality of what we're dealing with here in this issue and we should'n just slap a bandaid on something when we don't fully understand the workings of the code.

Try to keep the conversation without any personal comments, take this as a piece of feedback.
Such comments are not necessary ✌

[Krishna2323]

App/src/components/Pressable/GenericPressable/BaseGenericPressable.tsx

Lines 64 to 75 in 43d1576

	/**
	* Returns the cursor style based on the state of Pressable
	*/
	const cursorStyle = useMemo(() => {
	if (shouldUseDisabledCursor) {
	return styles.cursorDisabled;
	}
	if ([rest.accessibilityRole, rest.role].includes(CONST.ROLE.PRESENTATION)) {
	return styles.cursorText;
	}
	return styles.cursorPointer;
	}, [styles, shouldUseDisabledCursor, rest.accessibilityRole, rest.role]);

Even if you remove this cursorStyle, the pressable would still have the hand cursor because that's the default even if we don't pass any cursor style it will show the hand cursor, pls go ahead & try on. Sorry if it seems personal to you but for me it doesn't look like a personal comment, Its harsh but not a personal comment. Pls try removing that style and let me know if we are overriding or not. I'm literally frustrated because of the arguments and I still standby my point that you proposal is same as mine applying role to parent pressable or style to view has literally no difference, in both cases we are overriding. So I don't think its a major point to select you proposal.

[ikevin127]

Even if you remove this cursorStyle, the pressable would still have the hand cursor, pls go ahead & try on.

I am aware of this but we are not going to do that since this Pressable logic is currently used throughout the codebase.

This is why I said that we have to work with the current codebase logic and not against it by: 'what if we remove this logic ? then I would be right' kind of arguments as this would add redundant code while the root problem is still there.

I'm going to stop here as given all the context I think this is enough for the team to make an educated decision on the matter.
Cheers!

[aldo-expensify]

Drawing the line wether a proposal is different than another or not is pretty subjective, so I think we have to accept that what was decided may not be aligned with our opinion. It is true that all proposals in the end are changing a cursor style somehow, but that is unavoidable, so I don't think I still don't think adding role is different from adding styles on view is a strong argument here, and I don't think sharing the same RCA is reason enough either.

For me, the implementation using role is cleaner, it is a prop that we already have and seems to be exactly for what we want here. Adding cursor:'text'/styles.cursorText to textInputMultilineContainer doesn't align with its sibling style textInputContainer. I also believe there is a difference between correcting the style at the source compare to correcting it in a child component (correcting it at the source is better for me).

your proposal was not selected because you copied, sorry for being rude but you are trying to put redundant arguments to create confusions

Again, there is lots of subjective things in this conversation and decision, and I don't think this comment above is accurate neither constructive, so I would suggest avoiding such comments because it derails conversations into arguments that become personal (this does look like a personal attack to me). Keeping a good and friendly work environment is important for a long term working relationship, so I would recommend to not lose sight of that.

I think everything that was important to be said about why one proposal is better than the other has been said, the latest comments in this thread have very little value, so I ask you to refrain from continuing this discussion.

[melvin-bot]

📣 @ikevin127 🎉 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 📖

[Krishna2323]

@aldo-expensify, I apologize if this appears to be a personal remark, but I'd like to address that in the situation mentioned here, the payment was reduced to $250 and split among other members, despite the fact that the other two proposals were incomplete and likely to cause regressions. I did not contest their contributions. However, I feel that I invested time in identifying the root cause and providing a working solution before any other proposals were made. Consequently, I anticipated being assigned to this task in a similar manner.

Regardless, I'll consider it unfortunate luck for me. Thanks for your time 🙏🏻

[aldo-expensify]

@ikevin127 refrain with reacting with 👎 or such because it is not constructive and may be annoying to the commenter (or me reading it)

@Krishna2323 are you suggesting that we should split 250/250 because you contributed here and the other issues is an example of this outcome?

If that is your suggestion, I will decline that because: it is true that sometimes we do that, but I don't think that this should be the norm. I don't want everyone requesting this just because they posted the right RCA before or because in their opinion their solution is the same.

This is just how the rules currently are, sometimes you will spend time investigating and you just won't get rewarded, but hopefully some other issue reward will make up for it so you still feel it was worth it.

If you think the system should be improved, I think you should bring the conversation to the right slack channel.

despite the fact that the other two proposals were incomplete and likely to cause regressions

I disagree with that they would likely cause regressions

Regardless, I'll consider it unfortunate luck for me.

I think this is the best in this case, there is a lot in this that is out of your control. For example, if there was another internal engineer assigned, that engineer could have chosen your solution (same as @rushatgabhane) as best. Just keep in mind that I don't have a personal preference between you or @ikevin127 , I don't know any of you personally, I'm just making a decision considering what I think is fair and what I think is best for our codebase. Hopefully, in average, you shouldn't feel wronged because sometimes you will get lucky and sometimes someone else.

[Krishna2323]

@aldo-expensify, I didn't meant to split 250/250 😅, thanks this helps🙏🏻

I think this is the best in this case, there is a lot in this that is out of your control. For example, if there was another internal engineer assigned, that engineer could have chosen your solution (same as @rushatgabhane) as best. Just keep in mind that I don't have a personal preference between you or @ikevin127 , I don't know any of you personally, I'm just making a decision considering what I think is fair and what I think is best for our codebase. Hopefully, in average, you shouldn't feel wronged because sometimes you will get lucky and sometimes someone else.

[ikevin127]

PR #36286 ready for review! 🚀

Note

I passed down the role prop for the BaseTextInput/index.native.tsx too, for good measure ✌

[ikevin127]

Looks like the issue was fixed by this recently merged PR #35878 which was just deployed on staging, coming from this issue: #33893. I closed the PR and I think this issue is resolved 👏

cc @abekkala

[aldo-expensify]

I closed the PR and I think this issue is resolved 👏

Yeah, I checked too, it seems to be working fine now

[aldo-expensify]

Thank you all for work!