VBA -URL is shown on bottom left corner when `Fix the error ` is hovered reported by @huzaifa-99

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to /bank-account/BankAccountStep
2. Click on the first field, and then click outside, just to blur the field and force the validation error to be shown
3. Notice that an error label appears before submit button with blue coloured “fix the errors” text inside the label
4. Hover on “fix the errors” text
5. 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[bot]]

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

[hungvu193]

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. https://github.com/Expensify/App/blob/0d653cad15c32150c302ac12dc0acdbca1ece18b/src/components/TextLink.js#L60-L66 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:

https://user-images.githubusercontent.com/16502320/201486937-76c1a004-234a-4c51-a42f-4210b25e22b1.mov

[huzaifa-99]

Proposal

The issue has origins in FormAlertWrapper. We are using a TextLink there which display's the current url when hovered https://github.com/Expensify/App/blob/1b375ed3d3a7e1e42d8e746cb0e6f205ee0a3786/src/components/FormAlertWrapper.js#L59-L64

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

https://user-images.githubusercontent.com/68777211/201543329-45528a86-b6fb-464f-8f39-8376afd4be4e.mp4

[0xmiroslav]

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

[huzaifa-99]

I thought we are not handling accessibility as discussed here

[0xmiroslav]

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]

Updated Proposal (original https://github.com/Expensify/App/issues/12692#issuecomment-1312815631)

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 https://github.com/Expensify/App/blob/1b375ed3d3a7e1e42d8e746cb0e6f205ee0a3786/src/components/FormAlertWrapper.js#L57-L66 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]

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]

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

[huzaifa-99]

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

https://user-images.githubusercontent.com/16502320/201666709-4bba37ca-72e8-4c5a-879c-fa9c6900a78d.mov

@huzaifa-99 FYI

[huzaifa-99]

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)

https://user-images.githubusercontent.com/68777211/201677553-68c0b9a6-d274-4065-b93d-6e8f85a394bb.mp4

[hungvu193]

@huzaifa-99 Sure, here you are

[huzaifa-99]

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

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

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

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]

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]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @aldo-expensify (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[zanyrenney]

Hey @aldo-expensify - let me know what you think of these proposals and who we should hire.

[parasharrajat]

I am checking it...

[parasharrajat]

I do not consider this an issue. Why do we think this is an issue? an empty href value is completely valid just like href="#".

So href="" and href="#" is valid. It does not break anything for the user. @aldo-expensify Do you think we should fix it?

[aldo-expensify]

I'm pretty undecided about this, thoughts:

• Agree with that this is not necessarily an "issue", but I think the link showing in the corner has no value and if it can be removed easily without "workarounds", I think it is better to remove it.
• This has a very low ROI
• I tested the proposal by @mdneyazahmad, and it does seem to work without breaking accessibility (tab navigation still works)

If the proposal doesn't break other stuff, I think it would be fine to proceed with it, but it worries me a bit because it touches the default value for a prop of a widely used component.

If the mentioned proposal breaks something or doesn't work well, I think we should just close this issue because it is not worth spending more time for such low ROI

@parasharrajat what are your thoughts on the the proposal?

Update: some context related to the proposal... I see that the default value for href = '' was added when TextLink was using Pressable, but now we use Text, so maybe the default href = null makes more sense now? 🤷

Update 2: href prop doesn't seem to be in the documentation for the react native Text, this doesn't give me much confidence on relying on it for the solution.

[0xmiroslav]

Proposal

TextLink is widely used component in the app. So if this GH is really an issue, we can safely add href prop value in FormAlertWrapper rather than setting default prop of href in TextLink. This way, if href not set in TextLink, it goes to currently open page (same as #) as default (this is current behavior)

https://github.com/Expensify/App/blob/main/src/components/FormAlertWrapper.js#L59-L64

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

[JmillsExpensify]

If the mentioned proposal breaks something or doesn't work well, I think we should just close this issue because it is not worth spending more time for such low ROI

For what it's worth, this is where I'm at. If we can't come up with a proposal without side-effects, then I think we should close this issue.

[parasharrajat]

Thanks for the thoughts. Reviewing the proposals.

[parasharrajat]

I checked and It seems that defaultValue of null does not break the tab navigation. @mdneyazahmad can you confirm that this change won't break anything?