[HOLD for payment 2023-02-10] [$2000] Showing pointer cursor on code-block chat message instead of I-beam cursor #14301
Comments
kavimuru
added
Daily
Bug0 Triage ChecklistNote: see this SO for more information.
|
arielgreen
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~014c6f096c770787e9 |
|
Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @cead22 ( |
ProposalRCAThis issue is caused after the merging of PR #12987 . The use of the SolutionWe can specifically pass the diff --git a/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js b/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
index 25ea94e0ae..0e3946a273 100644
--- a/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
+++ b/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
@@ -6,6 +6,7 @@ import _ from 'underscore';
import htmlRendererPropTypes from '../htmlRendererPropTypes';
import withLocalize from '../../../withLocalize';
import {ShowContextMenuContext, showContextMenuForReport} from '../../../ShowContextMenuContext';
+import styles from '../../../../styles/styles';
const propTypes = {
/** Press in handler for the code block */
@@ -42,6 +43,7 @@ const BasePreRenderer = forwardRef((props, ref) => {
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
onLongPress={event => showContextMenuForReport(event, anchor, reportID, action, checkIfContextMenuActive)}
+ style={[styles.cursorAuto]}
>
{/* eslint-disable-next-line react/jsx-props-no-spreading */}
<TDefaultRenderer {...defaultRendererProps} />
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 71fed02015..6c163b940c 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -2360,6 +2360,10 @@ const styles = {
cursor: 'pointer',
},
+ cursorAuto: {
+ cursor: 'auto',
+ },
+
fullscreenCard: {
position: 'absolute',
left: 0,Results2023-01-19.04-20-44.mp4 |
ProposalWe can fix this by creating and using diff --git a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
index 456cd1ecf..999771e6c 100755
--- a/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
+++ b/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js
@@ -41,7 +41,7 @@ const customHTMLElementModels = {
}),
};
-const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText]};
+const defaultViewProps = {style: [styles.alignItemsStart, styles.userSelectText, styles.cursorAuto]};
// We are using the explicit composite architecture for performance gains.
// Configuration for RenderHTML is handled in a top-level component providing
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 71fed0201..1761c21db 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -2343,6 +2343,10 @@ const styles = {
marginVertical: 4,
},
+ cursorAuto: {
+ cursor: 'auto',
+ },
+
cursorDefault: {
cursor: 'default',
},Democursor.mp4 |
|
Thanks, guys for the proposals! Since this issue only occurs for the 🎀 👀 🎀 C+ reviewed! |
|
@mollfpr During testing, I noticed another issue I'd like to discuss. Trying to select a text enclosed in |
|
Update on above comment, turns out to be a new edge case that hasn't been reported. This issue is only occurring for desktops that have a touch screen. By default, selection is disabled for touch screen devices. This is targeted for the mobile platforms. We should discuss if this is something that should be within our scope of support. This issue will occur for all touch screen enabled desktops, wherever |
Proposal (kind of)Revert RCAThis is a regression by #12987 |
Proposal 2diff --git a/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js b/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
index 25ea94e0ae..e52aa318f6 100644
--- a/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
+++ b/src/components/HTMLEngineProvider/HTMLRenderers/PreRenderer/BasePreRenderer.js
@@ -1,6 +1,6 @@
import React, {forwardRef} from 'react';
import {ScrollView} from 'react-native-gesture-handler';
-import {Pressable} from 'react-native';
+import {TouchableWithoutFeedback, View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import htmlRendererPropTypes from '../htmlRendererPropTypes';
@@ -38,14 +38,16 @@ const BasePreRenderer = forwardRef((props, ref) => {
action,
checkIfContextMenuActive,
}) => (
- <Pressable
+ <TouchableWithoutFeedback
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
onLongPress={event => showContextMenuForReport(event, anchor, reportID, action, checkIfContextMenuActive)}
>
- {/* eslint-disable-next-line react/jsx-props-no-spreading */}
- <TDefaultRenderer {...defaultRendererProps} />
- </Pressable>
+ <View>
+ {/* eslint-disable-next-line react/jsx-props-no-spreading */}
+ <TDefaultRenderer {...defaultRendererProps} />
+ </View>
+ </TouchableWithoutFeedback>
)}
</ShowContextMenuContext.Consumer>
</ScrollView>RCAIn case my finding above is incorrect and that wrapping inside |
|
@mollfpr I think both solutions work, but I'm curious if you think using |
|
@s77rt Do we really need the extra @cead22 Tested I also notice that we have different feedback when long pressing on Screen.Recording.2023-01-20.at.08.48.09.movIf we use Screen.Recording.2023-01-20.at.08.53.11.mov |
|
I've got the PR ready, waiting to be assigned before creating it. |
|
@mollfpr If we don't wrap with |
|
|
|
None of the other renderers make use of a |
|
@Prince-Mendiratta Other renders do not use it because it makes sense there to use |
|
Thanks @Prince-Mendiratta @s77rt! Okay, after testing several times I couldn't find any issue with the For the highlight issue I mentioned before, turns out the Screen.Recording.2023-01-20.at.17.55.35.movYour call @cead22 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.64-7 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-02-10. 🎊 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.
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:
|
|
Regression Steps:
|
|
OOO this week -- reassigning so payment isn't held on me. |
|
Triggered auto assignment to @slafortune ( |
Bug0 Triage Checklist (Main S/O)
|
@mollfpr do you want to kick off a discussion about this? Perhaps we can everyone that when working with code blocks they should always make sure the cursor is the I-beam, or maybe we can add a new item to the reviwer checklist like: if you're updating code relating to rendering a code block, make sure the cursor on code blocks is still the I-beam cursor (or is that overkill?) Let me know what you think, and I'm also happy to start the convo in slack. Thanks! |
|
@cead22 Sorry, I missed this. Adding a new checklist will be overkill; it’s a rare case with updating the component. Let’s start a slack chat if you have another thoughts. |
|
@cead22 @slafortune start a discussion on Slack! |
|
@slafortune Accepted the offer, thank you! |
|
All paid - |
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:
testExpected Result:
On code-block message should show I-beam cursor as it not performing any action. We can copy it as normal text.
Actual Result:
Showing Pointer cursor on code-block messages.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.54-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
Notes/Photos/Videos:
pointer-cursor.mov
Recording.1283.mp4
Expensify/Expensify Issue URL:
Issue reported by: @jatinsonijs
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673621030750769
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: