[$250] VBA -URL is shown on bottom left corner when `Fix the error` is hovered reported by @huzaifa-99 #12692

kavimuru · 2022-11-12T02:45:08Z

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:

Navigate to /bank-account/BankAccountStep
Click on the first field, and then click outside, just to blur the field and force the validation error to be shown
Notice that an error label appears before submit button with blue coloured “fix the errors” text inside the label
Hover on “fix the errors” text
Notice it shows url address on bottom/left corner of the browser with the current url address set

Expected Result:

The label “fix-the-errors” should not show url-address when hovered

Actual Result:

“fix-the-errors” label is showing the current url as address on bottom-left screen corner when hovered

Workaround:

unknown

Platform:

Where is this issue occurring?

Web

Version Number: 1.2.27-3
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/201452993-388bb712-3673-4a28-822c-43aeb6623b99.mp4

Expensify/Expensify Issue URL:
Issue reported by: @huzaifa-99
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668124426446149

View all open jobs on GitHub

melvin-bot · 2022-11-12T02:45:13Z

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

hungvu193 · 2022-11-12T17:29:40Z

Proposal
In our current TextLink.js component, we are using the href props, that's why we see the bottom left url when the cursor hovered it.

App/src/components/TextLink.js

Lines 60 to 66 in 0d653ca

    
           <Text 
        
               style={[styles.link, ...additionalStyles]} 
        
               accessibilityRole="link" 
        
               href={props.href} 
        
               onPress={openLink} 
        
               onKeyDown={openLinkIfEnterKeyPressed} 
        
           >

Solution
To make it didn't show the bottom left url when hovered we can pass undefined as href prop. In some case, we need to use the TextLink.js but we didn't pass the href (like in this issue), we need to define the href prop default value.

        <Text
            style={[styles.link, ...additionalStyles]}
            accessibilityRole="link"
-           href={props.href} 
+           href={props.href ? props.href : undefined}
            onPress={openLink}
            onKeyDown={openLinkIfEnterKeyPressed}
        >
            {props.children}
        </Text>

or we can change the TextLink.js defaultProps:

const defaultProps = {
-   href:  '',
+   href: undefined,
    style: [],
    onPress: undefined,
};

Result:

Screen.Recording.2022-11-13.at.00.29.12.mov

huzaifa-99 · 2022-11-13T20:35:05Z

Proposal

The issue has origins in FormAlertWrapper. We are using a TextLink there which display's the current url when hovered

App/src/components/FormAlertWrapper.js

Lines 59 to 64 in 1b375ed

    
           <TextLink 
        
               style={styles.label} 
        
               onPress={props.onFixTheErrorsLinkPressed} 
        
           > 
        
               {props.translate('common.fixTheErrors')} 
        
           </TextLink>

Since we are not passing an actual link/href prop, this TextLink can be replaced with a normal Text component and adding link styles

<Text
    style={[styles.label, styles.link]}
    onPress={props.onFixTheErrorsLinkPressed}
>
    {props.translate('common.fixTheErrors')}
</Text>

which fixes the bug

Demo

12692.-.Demo.-.Hide.url.when.hovered.over.link.-.Screen.Recording.2022-11-14.at.1.32.53.AM.mp4

0xmiros · 2022-11-13T20:41:48Z

Your proposal will cause accessibility issue.
Cannot focus and click on fix the errors using TAB and Enter keyboard

huzaifa-99 · 2022-11-13T20:44:56Z

I thought we are not handling accessibility as discussed here

0xmiros · 2022-11-13T20:46:14Z

I thought we are not handling accessibility as discussed here

But that doesn't mean it's fine to break currently working accessibility feature

huzaifa-99 · 2022-11-13T20:57:12Z

Updated Proposal (original #12692 (comment))

If not breaking accessibility is a concern than we can wrap the Text in a Pressable and add the press callback there. And since it will be nested inside a Text it might break wrapping on native, so we can wrap it in a View.

Basically replacing this whole block

App/src/components/FormAlertWrapper.js

Lines 57 to 66 in 1b375ed

    
           <Text style={[styles.formError, styles.mb0]}> 
        
               {`${props.translate('common.please')} `} 
        
               <TextLink 
        
                   style={styles.label} 
        
                   onPress={props.onFixTheErrorsLinkPressed} 
        
               > 
        
                   {props.translate('common.fixTheErrors')} 
        
               </TextLink> 
        
               {` ${props.translate('common.inTheFormBeforeContinuing')}.`} 
        
           </Text>

with

<View style={{ flexDirection: 'row' }}>
    <Text style={[styles.formError, styles.mb0]}>
        {`${props.translate('common.please')} `}
    </Text>
    <Pressable onPress={props.onFixTheErrorsLinkPressed}>
        <Text style={[styles.label, styles.link]}>
            {props.translate('common.fixTheErrors')}
        </Text>
    </Pressable>
    <Text style={[styles.formError, styles.mb0]}>
        {` ${props.translate('common.inTheFormBeforeContinuing')}.`}
    </Text>
</View>

This also achieves the same result but accessibility is not broken

hungvu193 · 2022-11-14T11:33:01Z

This will cause an error console on web, you need to make sure there's no console error happened with your solution.

    <Pressable onPress={props.onFixTheErrorsLinkPressed}>
        <Text style={[styles.label, styles.link]}>
            {props.translate('common.fixTheErrors')}
        </Text>
    </Pressable>

mdneyazahmad · 2022-11-14T11:41:43Z

@huzaifa-99 are you able to reproduce the issue in dev? I can reproduce the issue in staging, but not in dev.

huzaifa-99 · 2022-11-14T12:24:14Z

@hungvu193 i see no console errors specific to this change. Can you please describe a little more? Thanks

@mdneyazahmad yes still reproducible on v1.2.27-3 (DEV)

hungvu193 · 2022-11-14T13:05:03Z

Screen.Recording.2022-11-14.at.20.00.58.mov

@huzaifa-99 FYI

huzaifa-99 · 2022-11-14T13:57:54Z

Hm strange, I don't get this error. @hungvu193 Can you please expand the log to see the stack trace. Thanks in advance

For a reference please see this demo (ignore the 30x git file changes in my vscode, they are just console logs)

No.console.errors.on.fix.Screen.Recording.2022-11-14.at.6.52.31.PM.mp4

hungvu193 · 2022-11-14T14:41:50Z

@huzaifa-99 Sure, here you are

huzaifa-99 · 2022-11-14T14:52:30Z

@hungvu193 I see where the problem is.

In your video demo, you are using this code

<Pressable 
    style={[styles.label, styles.link]}
    onPress={props.onFixTheErrorsLinkPressed}
>    
    {props.translate('common.fixTheErrors')}
</Pressable>

but in your comment here, it was different. That's why you get the error because {props.translate('common.fixTheErrors')} should be wrapped in a Text element

Again see my proposal here for the correct code block

hungvu193 · 2022-11-14T15:04:09Z

@hungvu193 I see where the problem is.

In your video demo, you are using this code
<Pressable 
    style={[styles.label, styles.link]}
    onPress={props.onFixTheErrorsLinkPressed}
>    
    {props.translate('common.fixTheErrors')}
</Pressable>
but in your comment here, it was different. That's why you get the error because {props.translate('common.fixTheErrors')} should be wrapped in a Text element

Again see my proposal here for the correct code block

ah I see.

mdneyazahmad · 2022-11-14T15:18:51Z

Proposal

If href property is not null, a is rendered. So pass null for non href link. Also add focusable prop to focus on tab navigation when it is not a href link.

diff --git a/src/components/TextLink.js b/src/components/TextLink.js
index 55344d987..3aa2aaf25 100644
--- a/src/components/TextLink.js
+++ b/src/components/TextLink.js
@@ -58,9 +58,10 @@ const TextLink = (props) => {
 
     return (
         <Text
+            focusable
             style={[styles.link, ...additionalStyles]}
             accessibilityRole="link"
-            href={props.href}
+            href={props.href ? props.href : null}
             onPress={openLink}
             onKeyDown={openLinkIfEnterKeyPressed}
         >

OR

diff --git a/src/components/TextLink.js b/src/components/TextLink.js
index 55344d987..bff36764b 100644
--- a/src/components/TextLink.js
+++ b/src/components/TextLink.js
@@ -25,7 +25,7 @@ const propTypes = {
 };
 
 const defaultProps = {
-    href: '',
+    href: null,
     style: [],
     onPress: undefined,
 };
@@ -58,6 +58,7 @@ const TextLink = (props) => {
 
     return (
         <Text
+            focusable
             style={[styles.link, ...additionalStyles]}
             accessibilityRole="link"
             href={props.href}

Can somebody test this for me. I am not able to reproduce on my macbook.

melvin-bot · 2022-11-14T15:18:54Z

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

mdneyazahmad · 2022-11-14T15:37:27Z

Updated Proposal

This fixes the issue

diff --git a/src/components/TextLink.js b/src/components/TextLink.js
index 55344d987..a453b9788 100644
--- a/src/components/TextLink.js
+++ b/src/components/TextLink.js
@@ -25,7 +25,7 @@ const propTypes = {
 };
 
 const defaultProps = {
-    href: '',
+    href: null,
     style: [],
     onPress: undefined,
 };

Thanks @huzaifa-99 I can reproduce the issue in DEV.

zanyrenney · 2022-11-14T17:44:44Z

The job listing is here: https://www.upwork.com/jobs/~01bd917604edb70bce

melvin-bot · 2022-11-15T13:31:23Z

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2022-11-15T13:31:32Z

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

zanyrenney · 2022-11-24T12:15:53Z

I am having troubles logging in to Upwork just now:

I'll hire you when I get it sorted, sorry @hungvu193

hungvu193 · 2022-11-24T12:19:05Z

I am having troubles logging in to Upwork just now:

I'll hire you when I get it sorted, sorry @hungvu193

no worries :)

zanyrenney · 2022-11-24T13:15:37Z

I got back in and have hired you @hungvu193 - let me know if you have any further questions :)

hungvu193 · 2022-11-24T13:35:44Z

I got back in and have hired you @hungvu193 - let me know if you have any further questions :)

Thank you, I accepted your offer :)

huzaifa-99 · 2022-11-24T14:07:09Z

@zanyrenney am I eligible for reporting on this?

zanyrenney · 2022-11-24T15:02:45Z

Yes! I have hired you there too @huzaifa-99 :)

melvin-bot · 2022-11-28T08:03:37Z

@hungvu193, @parasharrajat, @zanyrenney, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

zanyrenney · 2022-11-28T14:39:00Z

Hired contributors, waiting on the reviews @aldo-expensify @parasharrajat

parasharrajat · 2022-11-28T14:59:29Z

PR deployed to PROD 5 days back. #12858 (comment)

zanyrenney · 2022-11-29T16:35:42Z

Waiting until the 7 days and then we will be ready for payment.

zanyrenney · 2022-12-02T10:22:15Z

sent the payment to you both on upwork @hungvu193 @huzaifa-99 - any questions, lmk!

hungvu193 · 2022-12-02T10:25:04Z

@zanyrenney Since the PR was merged within 3 days, Can I have the bonus as well?

parasharrajat · 2022-12-04T15:05:30Z

@zanyrenney Could you please send me offer for C+?

aldo-expensify · 2022-12-08T22:33:27Z

Friendly bump @zanyrenney

zanyrenney · 2022-12-12T17:20:05Z

have also extended the contract to you @parasharrajat - please accept it so I can pay it out.

@hungvu193 bonus has been paid.

parasharrajat · 2022-12-12T18:47:39Z

@zanyrenney Accepted.

zanyrenney · 2022-12-15T18:37:13Z

@parasharrajat paid, inc bonus! closing this issue out now as everyone has been paid.

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2022

melvin-bot bot assigned zanyrenney Nov 12, 2022

melvin-bot bot added the Overdue label Nov 14, 2022

melvin-bot bot removed the Overdue label Nov 14, 2022

zanyrenney added the External Added to denote the issue can be worked on by a contributor label Nov 15, 2022

melvin-bot bot assigned parasharrajat Nov 15, 2022

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2022

melvin-bot bot assigned aldo-expensify Nov 15, 2022

melvin-bot bot removed the Overdue label Nov 24, 2022

melvin-bot bot added the Overdue label Nov 28, 2022

melvin-bot bot removed the Overdue label Nov 28, 2022

melvin-bot bot added the Overdue label Dec 2, 2022

melvin-bot bot removed the Overdue label Dec 2, 2022

melvin-bot bot added the Overdue label Dec 8, 2022

melvin-bot bot added Overdue and removed Overdue labels Dec 8, 2022

melvin-bot bot removed the Overdue label Dec 12, 2022

melvin-bot bot added the Overdue label Dec 15, 2022

zanyrenney closed this as completed Dec 15, 2022

melvin-bot bot added Overdue and removed Overdue labels Dec 15, 2022

[$250] VBA -URL is shown on bottom left corner when Fix the error is hovered reported by @huzaifa-99 #12692

[$250] VBA -URL is shown on bottom left corner when Fix the error is hovered reported by @huzaifa-99 #12692

Comments

kavimuru commented Nov 12, 2022

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

melvin-bot bot commented Nov 12, 2022

hungvu193 commented Nov 12, 2022 • edited Loading

huzaifa-99 commented Nov 13, 2022 • edited Loading

Proposal

Demo

0xmiros commented Nov 13, 2022

huzaifa-99 commented Nov 13, 2022

0xmiros commented Nov 13, 2022

huzaifa-99 commented Nov 13, 2022

Updated Proposal (original #12692 (comment))

hungvu193 commented Nov 14, 2022 • edited Loading

mdneyazahmad commented Nov 14, 2022

huzaifa-99 commented Nov 14, 2022 • edited Loading

hungvu193 commented Nov 14, 2022

huzaifa-99 commented Nov 14, 2022 • edited Loading

hungvu193 commented Nov 14, 2022

huzaifa-99 commented Nov 14, 2022

hungvu193 commented Nov 14, 2022

mdneyazahmad commented Nov 14, 2022 • edited Loading

Proposal

OR

melvin-bot bot commented Nov 14, 2022

mdneyazahmad commented Nov 14, 2022

Updated Proposal

zanyrenney commented Nov 14, 2022

melvin-bot bot commented Nov 15, 2022

melvin-bot bot commented Nov 15, 2022

zanyrenney commented Nov 24, 2022

hungvu193 commented Nov 24, 2022

zanyrenney commented Nov 24, 2022

hungvu193 commented Nov 24, 2022

huzaifa-99 commented Nov 24, 2022

zanyrenney commented Nov 24, 2022

melvin-bot bot commented Nov 28, 2022

zanyrenney commented Nov 28, 2022

parasharrajat commented Nov 28, 2022

zanyrenney commented Nov 29, 2022

zanyrenney commented Dec 2, 2022

hungvu193 commented Dec 2, 2022

parasharrajat commented Dec 4, 2022

aldo-expensify commented Dec 8, 2022

zanyrenney commented Dec 12, 2022

parasharrajat commented Dec 12, 2022

zanyrenney commented Dec 15, 2022

[$250] VBA -URL is shown on bottom left corner when `Fix the error` is hovered reported by @huzaifa-99 #12692

[$250] VBA -URL is shown on bottom left corner when `Fix the error` is hovered reported by @huzaifa-99 #12692

hungvu193 commented Nov 12, 2022 •

edited

Loading

huzaifa-99 commented Nov 13, 2022 •

edited

Loading

hungvu193 commented Nov 14, 2022 •

edited

Loading

huzaifa-99 commented Nov 14, 2022 •

edited

Loading

huzaifa-99 commented Nov 14, 2022 •

edited

Loading

mdneyazahmad commented Nov 14, 2022 •

edited

Loading