[$500] [BUG] - Url isn't fully visible in the tooltip reported by @thesahindia

[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 any chat
2. Send a long url (e.g. https://www.expensify.com/inbox?policyID=D189E4B5582EAE8D&taskID=AddWorkEmail)
3. Hover

Expected Result:

The whole Url should be visible

Actual Result:

The Url isn't fully visible

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.2.10-0 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:

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

View all open jobs on GitHub

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@tjferriss Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

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

[NikkiWines]

Hmm, this is because we have an intentional maxWidth restriction for tooltips. I'd assume we wouldn't want to have that dynamically sized based on the length of the link. That could look really weird if the link is super long...

Theoretically we could do something like a scrolling tooltip but I'm not sure it's really necessary or that it's a bug that it's cut off and doesn't scroll. cc: @shawnborton for your design expertise and since you added the original maxWidth restriction here (it's used here)

[parasharrajat]

This feels like a regression. Tooltip content should wrap in next lines after max width is reached.

[NikkiWines]

Ohh, yes, you're right @parasharrajat - looks like max lines should be 3 but it's not wrapping. That still feels (to me) like it would be bulky but I think you're right that it's a regression

[NikkiWines]

Given that, marking this as external so we can get someone working on this.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[shawnborton]

Personally I think it this is fine as it is with the max-width, but perhaps we should truncate via ellipsis instead of just a hard cut off.

[shawnborton]

But actually, seeing your comment above:

Ohh, yes, you're right @parasharrajat - looks like max lines should be 3 but it's not wrapping. That still feels (to me) like it would be bulky but I think you're right that it's a regression

I agree with that

[NikkiWines]

@shawnborton, to be clear, you're suggesting we keep it to a single line but truncate with an elipsis?

[shawnborton]

At first I was, but then I saw your comment that this is actually a regression and we should go back to the way it as before with the text wrap

[aimane-chnaif]

This is a regression from https://github.com/Expensify/App/pull/10504 Reverting this PR fixes this GH but safari wrapped issue (Un / pin) still remaining

[sobitneupane]

@aimane-chnaif We would want this issue to be fixed without creating the other issue. So, Proposal that fixes this issue without affecting other would be ideal.

[bfitzexpensify]

Upwork job here

Reporting job here @thesahindia

[danishyasin33]

Hi everyone,

The issue seems to have something to do with how the width of the tooltip is calculated. If I remove the the width or set it to fit-content then the URL shows it's full length.

The Fix:

To fix it, we can fix the width attribute of the tooltip for it to appear correctly.

Or, if the team wants that the tooltip is wrapped after some fix width, say 400px, then we'll have to add a wrap attribute around the span of the tooltip. Something like this:

Whatever the team decides, I can make that fix and should be a quick process.

[sobitneupane]

Hello @danishyasin33, Can you please point out the file where you proposing to make change and what are the changes you are proposing? Some codeblocks will be helpful. For example: https://github.com/Expensify/App/issues/11388#issuecomment-1262745992

If you haven’t already, check out CONTRIBUTING GUIDELINES.

[fedirjh]

hello there

This issue is related to white Space : it is set to nowrap which suppresses line breaks within the source text . to fix this you set white-space:normal ; and add word-break: break-all; to prevent overflow

component to edit is getTooltipStyles.js

[danishyasin33]

Hi @sobitneupane,

So the file I would edit would be: https://github.com/Expensify/App/blob/dfb5bca05a5b12e62ec86520819fd5a4ac66e405/src/styles/getTooltipStyles.js#L160-L161

I would update it to:

            overflow: 'hidden',
+          whiteSpace: 'break-spaces',

I believe this would fix the said issue if we are going for the line-break course of action.

[danishyasin33]

@sobitneupane if my proposed fix is satisfactory, could I be assigned this job please. 😄

[sobitneupane]

@danishyasin33 @fedirjh Thanks for the proposal.

Both the proposals have some issues. Changes suggested will recreate the issue fixed by: https://github.com/Expensify/App/pull/10504 Solution of this issue should not recreate the other issue.

[fedirjh]

@sobitneupane thanks for the update.

It looks like safari is still supporting the old alias of overflow-wrap which is word-wrap . here is an updated solution that works on safari and chrome . Tested on Safari and Chrome

changes made

-          whiteSpace: 'nowrap', 
+          whiteSpace: 'normal',
+          wordWrap: 'normal', 

on

https://github.com/Expensify/App/blob/dfb5bca05a5b12e62ec86520819fd5a4ac66e405/src/styles/getTooltipStyles.js#L161

Edit : Demo

https://user-images.githubusercontent.com/36869046/195436822-e5da61da-647f-4ec6-bb09-7da9f8f15c4f.mp4

[danishyasin33]

Hi @sobitneupane,

I tested it both Chrome and Safari after adding white-space: break-spaces, it does not recreate the issue fixed by #10504.

Here's a demo on both safari and chrome.

Chrome:

https://user-images.githubusercontent.com/47951155/195449997-70e9a5ea-031f-4441-b4eb-cb03fa0d15b4.mov

Safari:

https://user-images.githubusercontent.com/47951155/195450328-6cc63406-cc4c-4f27-a0ec-f85d69a57cc1.mov

As such, I don't see how it would cause regression and the original solution I proposed should be sufficient in the resolution of this error.

[sobitneupane]

@fedirjh @danishyasin33 @aimane-chnaif

The issue is existent not only on safari but also on other browsers.

https://user-images.githubusercontent.com/25876548/195486788-b029ef12-18c9-4ad2-94c2-fa9d62cb91d8.mov

[danishyasin33]

@sobitneupane you're right about the unintended consequences. To fix this, we'll need to pass a prop down from the BaseAnchorForCommentsOnly file where we'll define the TooltipType as commentLink.

This will be passed down all the way to getTooltipStyles and whitespace style will be determined by the following condition: const tooltipWhiteSpace = tooltipType == 'commentLink' ? 'break-spaces' : 'nowrap'

BaseAnchorForCommentsOnly.js:

          <Tooltip 
+           tooltipType="commentLink" 
            text={Str.isValidEmail(props.displayName) ? '' : props.href}>
                <Text
                    ref={el => linkRef = el}
                    style={StyleSheet.flatten([props.style, defaultTextStyle])}
                    accessibilityRole="link"
                    hrefAttrs={{
                        rel: props.rel,
                        target: props.target,
                    }}
                    // eslint-disable-next-line react/jsx-props-no-spreading
                    {...linkProps}
                    // eslint-disable-next-line react/jsx-props-no-spreading
                    {...rest}
                >
                    {props.children}
                </Text>
            </Tooltip>

getTooltipStyles.js:

.
.
.
 * @param {Number} [manualShiftVertical] - Any additional amount to manually shift the tooltip up or down.
 *                                       A positive value shifts it down, and a negative value shifts it up.
+ * @param {String} [tooltipType] - Defines the type of tool tip
 * @returns {Object}
 */

.
.
.

+    // We get the tooltip WhiteSpace style by determining which type of tooltip it is.
+    const tooltipWhiteSpace = tooltipType == 'commentLink' ? 'break-spaces' : 'nowrap'

.
.
.

        tooltipTextStyle: {
            color: themeColors.textReversed,
            fontFamily: fontFamily.GTA,
            fontSize: tooltipFontSize,
            overflowWrap: 'normal',
            overflow: 'hidden',
+          whiteSpace: tooltipWhiteSpace,
            left: '1px',
        },

This keeps the change limited to just the tooltips of the comments. Avoid any unnecessary regression.

Demo (desktop):

https://user-images.githubusercontent.com/47951155/195546784-99e4bf20-2f03-4c6c-bc4b-06da70d1c5b2.mov

This is in chrome, the issue resolves from Safari as well, I checked.

[fedirjh]

@sobitneupane the problem with white-space: nowrap; , it always suppresses line breaks always even when width is set , i have 2 solution

Solution 1

Since we have the both tooltipTextWidth and maxWidth . we can set dynamically the white-space to no-wrap if tooltipTextWidth < maxWidth else if tooltipTextWidth exceeds the max width we set white-space to normal (or break-spaces)

https://github.com/Expensify/App/blob/dfb5bca05a5b12e62ec86520819fd5a4ac66e405/src/styles/getTooltipStyles.js#L155-L162

-whiteSpace: 'nowrap'
+whiteSpace: tooltipTextWidth && tooltipTextWidth > maxWidth ? 'normal' : 'nowrap'

Solution 2

After we measure the tooltipTextWidth , we update the whitespace property. white-space should be set to nowrap on getTooltipStyles.js .

https://github.com/Expensify/App/blob/dfb5bca05a5b12e62ec86520819fd5a4ac66e405/src/components/Tooltip/TooltipRenderedOnPageBody.js#L83-L87

updateTooltipTextWidth() {
        this.setState({
            tooltipTextWidth: this.textRef.offsetWidth,
        });
+        this.textRef.style.whiteSpace = 'normal'
    }

then we add a key to the tooltipText

https://github.com/Expensify/App/blob/dfb5bca05a5b12e62ec86520819fd5a4ac66e405/src/components/Tooltip/TooltipRenderedOnPageBody.js#L128

-<Text style={tooltipTextStyle} ref={ref => this.textRef = ref}>{this.props.text}</Text>
+<Text style={tooltipTextStyle} key={this.props.text} ref={ref => this.textRef = ref}>{this.props.text}</Text>

[danishyasin33]

Hi @sobitneupane,

Is the provided solution (https://github.com/Expensify/App/issues/11486#issuecomment-1277250267) satisfactory?

[sobitneupane]

@danishyasin33 @fedirjh

Proposals above might fix the issue. But both of them look quite hacky to me.

I believe the best course of action will be to remove whiteSpace: 'nowrap', line which cause this issue(introduced in https://github.com/Expensify/App/pull/10504 PR to solve issue mentioned here) and find a way to resolve the issue the PR was trying to solve.

cc: @thienlnam