[HOLD for payment 2022-05-20][$1000] Feature Request: Show link when hover over hyperlink

[mvtglobally]

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:

1. Navigate to chat
2. Send any text hyperlinked
3. Hover over or hard press over link

Expected Result:

Some tool tip displayed so that, when the hyperlink is hovered over, the destination link shows.

Actual Result:

Nothing happens on hover

Problem:

Currently users can use markdown to add hyperlinks to chats. The person who receives the link can't easily view the link to know where goes (inc. potential phishing links)

Solution:

Add a tool tip so that, when the hyperlink is hovered over, the destination link shows.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.39-1
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @mallenexpensify
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644366109776009

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @kadiealexander (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[sobitneupane]

PROPOSAL
To achieve this we need to make some changes in src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 54 to 70 in d9355db

	<AnchorForCommentsOnly
	href={attrHref}
	isAuthTokenRequired={isAttachment}
	// Unless otherwise specified open all links in
	// a new window. On Desktop this means that we will
	// skip the default Save As... download prompt
	// and defer to whatever browser the user has.
	// eslint-disable-next-line react/jsx-props-no-multi-spaces
	target={htmlAttribs.target || '_blank'}
	rel={htmlAttribs.rel || 'noopener noreferrer'}
	style={{...props.style, ...parentStyle}}
	key={props.key}
	fileName={fileName}
	>
	<TNodeChildrenRenderer tnode={props.tnode} />
	</AnchorForCommentsOnly>

<Tooltip text={attrHref}>
    <AnchorForCommentsOnly
        href={attrHref}
        isAuthTokenRequired={isAttachment}

        // Unless otherwise specified open all links in
        // a new window. On Desktop this means that we will
        // skip the default Save As... download prompt
        // and defer to whatever browser the user has.
        // eslint-disable-next-line react/jsx-props-no-multi-spaces
        target={htmlAttribs.target || '_blank'}
        rel={htmlAttribs.rel || 'noopener noreferrer'}
        style={{...props.style, ...parentStyle}}
        key={props.key}
        fileName={fileName}
    >
        <TNodeChildrenRenderer tnode={props.tnode} />
    </AnchorForCommentsOnly>
</Tooltip>

Output:

[parasharrajat]

This is an extra feature. it could be good to have but I don't it is needed at this stage.

[kadiealexander]

Agree, but if it's an easy fix I say we go for it. @mallenexpensify is right that it poses a small security risk to click on a link when you're not sure of the destination.

[MelvinBot]

Triggered auto assignment to @sketchydroide (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[parasharrajat]

Then I don't think a tooltip is best suited bfor this. Like other websites, hovering over the link the browser should show the link address on the cornor of the browser.

But this is not happening in our app so we should do that even though we are planning to show tooltip.

[sketchydroide]

slack seem to use a tool tip:

Althoug the mobile version doesn't show anything.

I think this is an external

[sketchydroide]

Then I don't think a tooltip is best suited bfor this. Like other websites, hovering over the link the browser should show the link address on the cornor of the browser.

But this is not happening in our app so we should do that even though we are planning to show tooltip.

I think for now let's show the tooltip, this is what slack seems to do, and for the rest of the none web based platforms I think it makes more sense.

[MelvinBot]

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

[sketchydroide]

😄 @mallenexpensify you have been chosen I was goint to tag you, but seems like it is no longer necessary

[parasharrajat]

But still Tooltips are only web thing.

[mallenexpensify]

Full circle! @parasharrajat is there another term we should use to denote what we're calling/considering a tooltip on Desktop?

We definitely want a mobile/app option too, right now there is no way to know where a hyperlink will send you.

How about, when someone taps then holds any link (hyperlinks and other domain links) we show the destination link above or below 'copy URL to clipboard`? Reasoning is because it's possible to use markdown to have a visible link go to a different site

Alt option would be to never allow links to go to other sites.

[parasharrajat]

Sorry for the confusion @mallenexpensify. Tooltip works on both web and desktop but not on the native apps (Android|IOS).

[MelvinBot]

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

[MelvinBot]

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

[mallenexpensify]

Hired @eVoloshchak and assigned to GH!

[eVoloshchak]

@parasharrajat
I've encountered a problem on Android (haven't tested on iOS yet, but pretty sure it is present there too).
When we have a message with text and a link, long pressing on text will pop up the context menu for the message (which is correct), but long pressing on the link will also pop up the context menu for the message (which is not correct, it should be a context message with Copy URL to clipboard). While if you have a message with just a link, It works correctly.
Here's a video to illustrate

22-04-28-13-14-38.mp4

The issue occurs because if PressableWithSecondaryInteraction is inline, we use Text instead of Pressable and it somehow interferes with LongPressGestureHandler

const Node = props.inline ? Text : Pressable;

I tried to fix this and found out, that if we completely remove LongPressGestureHandler and add onLongPress handler to Node, it works just fine

22-04-28-13-24-49.mp4

So this file would look like

const PressableWithSecondaryInteraction = (props) => {
    // Use Text node for inline mode to prevent content overflow.
    const Node = props.inline ? Text : Pressable;
    return (
        <Node
            ref={props.forwardedRef}
            onPress={props.onPress}
            onPressIn={props.onPressIn}
            onPressOut={props.onPressOut}
            onLongPress={(e) => {
                e.preventDefault();
                HapticFeedback.trigger();
                props.onSecondaryInteraction(e);
            }}
            {...props}
        >
            {props.children}
        </Node>
    );
};

Since you were the author of the PR that introduced this change, I thought you might know more.
Is approach I proposed acceptable?
Do we need LongPressGestureHandler?
I poked around the app and it doesn't seem to break anything, but maybe you have more context
Thanks!

[parasharrajat]

Hey, @eVoloshchak Thanks for looking into it. It is a different issue than the one we are trying to solve here. So you don't have to fix that in this PR.

we need to use Text for inline mode and we also need the LongPressGestureHandler as it fixes one of the old issues.

but we have a separate logged issue for the problem you mentioned and if you want to solve it, head over to #8311. I am happy to hear the alternatives there.

[eVoloshchak]

Hey, @eVoloshchak Thanks for looking into it. It is a different issue than the one we are trying to solve here. So you don't have to fix that in this PR.

Thanks, go it.

I'm ready to submit the PR, but it's gonna be impossible for the QA to test it for mWeb due to #8311.
Should I submit it or wait for this issue to be resolved?

[parasharrajat]

You can submit it and we can wait for the other one to be solved. It seems that only you have shared a proposal on the other one.

[melvin-bot]

@sketchydroide, @tgolen, @eVoloshchak, @mallenexpensify, @parasharrajat Huh... This is 4 days overdue. Who can take care of this?

[parasharrajat]

PR under review

[sketchydroide]

no update

[mallenexpensify]

This should be a Weekly, updated. The PR is moving along, should hit production soon
#8815

[trjExpensify]

👋 looks like this didn't quite work completely, issue reported with mWeb: #8960 (comment)

[eVoloshchak]

Issue isn't caused by this PR, more details here.

[mallenexpensify]

Paid @eVoloshchak , it's working on web and desktop (Version 1.1.64-0). No comments on the PR the past week.

@parasharrajat , hired you for C+ payment, can you accept the offer?

[parasharrajat]

@mallenexpensify Done.

[mallenexpensify]

Thanks @parasharrajat , paid $1000 for C+.