[HOLD for payment 2023-06-30] [Hold 18658][$1000] When only link is added in header text, app adds extra padding above link which has cursor as pointer

[kavimuru]

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. Open the app
2. Open any report
3. Send any link as header text. eg: # google.com
4. Hover slightly over the link and observe the pointer is cursor i.e. pointer is clickable.
5. Click in that area and observe that it does nothing

Expected Result:

App should only keep the link area clickable like does for normal text message and header message with both text and link

Actual Result:

App keeps area above link clickable but does nothing on click when only link is added in header text

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.0 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: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/232342197-70444093-fc37-4d9a-acc6-b8c19999e4e0.mp4

https://user-images.githubusercontent.com/43996225/232342200-4bb59a97-95eb-477e-b198-b3b3bd51e6c8.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681575226770749

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0143b139f54244d059
• Upwork Job ID: 1648636657245667328
• Last Price Increase: 2023-05-10

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0143b139f54244d059

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[flaviadefaria]

External label added, waiting for proposals.

[MahmudjonToraqulov]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When only link is added in header text, app adds extra padding above link which has cursor as pointer

What is the root cause of that problem?

display: inline should be removed from h1 's third child.

RULE: display: inline -> It just takes up the space such an element would normally take. Because of this, you can't set the width and height of an element that has a display of inline , becuase it does not take up the whole screen width.

In that case The element shouldn't be like rule

What changes do you think we should make in order to solve the problem?

Just remove display: inline from the element. More in the video.

What alternative solutions did you explore? (Optional)

N/A

Result

screen-capture (22).webm

[alexxxwork]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When h1 applied to link there's an area above with pointer cursor. App should only keep the link area clickable like does for normal text message and header message with both text and link

What is the root cause of that problem?

The root cause is here: https://github.com/Expensify/App/blob/c144663fcad41a69344311a27dd6a147a28e7191/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L68

What changes do you think we should make in order to solve the problem?

We could just delete containerStyles prop

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When only link is added in header text, app adds extra padding above link which has cursor as pointer

What is the root cause of that problem?

containerStyles with style display: inline is apply to container of text. When set display: inline, line-height: 20px doesn't work so that div that wrap text have height more than 20px. So that height of it is more than normal. In addition, h1 tag has contentModel is block, so that fontSize, fontFamily or other similar that is invalid for it self, it is applied for children of h1. In web, h1 has default fontSize is 2em when children has height higher it set width to 2em that is higher than 20px https://github.com/Expensify/App/blob/c144663fcad41a69344311a27dd6a147a28e7191/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L68

What changes do you think we should make in order to solve the problem?

We should remove containerStyles in this https://github.com/Expensify/App/blob/c144663fcad41a69344311a27dd6a147a28e7191/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L68

What alternative solutions did you explore? (Optional)

We could create new h1 tag like this: h1: defaultHTMLElementModels.div.extend({}), in customHTMLElementModels to change h1 to div when it is renderer and prevent the default style of h1 in the browser. We also could create a custom render for h1 tag to prevent the default style.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

App keeps area above link clickable but does nothing on click when only link is added in header text

What is the root cause of that problem?

The Pressable area in https://github.com/Expensify/App/blob/d42b51b3ad5bde56882979cef4157a1f71dc6ac2/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L31 is 20px, larger than the Pressable area of the a tag.

This is due to the comment div that is rendered does not have display: inline, so the pressable div captures the whole area which is 20px, while the children div all has display: inline from the Tooltip here https://github.com/Expensify/App/blob/c144663fcad41a69344311a27dd6a147a28e7191/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L68, so it only captures 18px which is what's needed to render the a tag.

What changes do you think we should make in order to solve the problem?

We need to add display: inline to the comment div so that the Pressable height will show correctly (18px). This is correct since the div renders text content so it should have display: inline style.

What alternative solutions did you explore? (Optional)

N/A

[alitoshmatov]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When only link is added in header text, app adds extra padding above link which has cursor as pointer

What is the root cause of that problem?

We are wrapping anchor element with tooltip and pressable.They are getting their height based on parent element styling because they are inline element https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L52-L88

Since we are supplying react-native-render-html with custom component for anchor element, it is just passing the styles to child element. It is not applying styles fully to the header element, and omitting some fields. Even though we have declared styles for h1 element https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/styles/styles.js#L134-L137 font-size is not applied to h1 itself and rather supplied to child element renderer. I think this is explained on their doc here https://meliorence.github.io/react-native-render-html/docs/flow/css-processing

Thus h1 element is defaulting to browser default styles and letting child elements to expand

The issue

What changes do you think we should make in order to solve the problem?

We should also apply styles to top level component of our custom anchor component, specifically apply styles to pressable component here https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L52-L55

What alternative solutions did you explore? (Optional)

[flaviadefaria]

@0xmiroslav any thoughts on the proposal above?

[flaviadefaria]

@0xmiroslav friendly bump here...

[0xmiros]

@MahmudjonToraqulov thanks for your proposal. I am not sure where you're proposing to remove display: inline. Can you please share permal link?

[0xmiros]

@alexxxwork can you please explain more detail why display: inline is the root cause?

[0xmiros]

@dukenv0307 the root cause is reasonable but unfortunately we cannot remove display: inline. It was added intentionally in #12802 to fix #11735

[0xmiros]

@alitoshmatov thanks for the root cause analysis. I agree with it but not solution. I don't like the idea of applying Text styles to parent view.

[0xmiros]

Note to contributors: display: inline removal might be a direct solution but instead we should find a way to fix https://github.com/Expensify/App/issues/11735

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dukenv0307]

@0xmiroslav I just saw that h1 tag doesn't apply fontSize: 17px style for it self and it has font-size: 2em by default of browser, while margin-bottom style is applied for it self and I'm finding the reason for this. It is also reasonable why only h1 with only link has this issue

[0xmiros]

@0xmiroslav I just saw that h1 tag doesn't apply fontSize: 17px style for it self and it has font-size: 2em by default of browser, while margin-bottom style is applied for it self. It is also resonable why only h1 with only link has this issue

yes correct. It's explained by @alitoshmatov in root cause.

[alitoshmatov]

@0xmiroslav But when there are link and some text, all styles are being applied to wrapper itself rather than applying it to h1 by react-native-render-html. So I think it is normal thing to do.

[dukenv0307]

@0xmiroslav I think the reason that h1 doesn't apply fontSize for it self because it is a block. So that fontSize, fontFamily and other similar that is invalid for it self and it has a children node that is a div element in web and all styles same as fontSize, fontFamily is applied for children. I tried to change it to mixed and with this change h1 element is rendered with no children and it is applied all styles for it self.

[MahmudjonToraqulov]

@MahmudjonToraqulov thanks for your proposal. I am not sure where you're proposing to remove display: inline. Can you please share permal link?

Hi @0xmiroslav . Yes, here: https://github.com/Expensify/App/blob/c144663fcad41a69344311a27dd6a147a28e7191/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js#L68

containerStyles={[styles.dInline]} should be removed

[0xmiros]

Thanks @MahmudjonToraqulov but still same feedback as https://github.com/Expensify/App/issues/17488#issuecomment-1523637950 so not acceptable.

Can you update your proposal based on https://github.com/Expensify/App/issues/17488#issuecomment-1523653757?

[uttarkhandcool]

nice

[MelvinBot]

📣 @uttarkhandcool! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[MahmudjonToraqulov]

but instead we should find a way to fix

Got it @0xmiroslav

[dukenv0307]

@0xmiroslav I just update my proposal with an other way to fix this issue by change contentModel of h1 to HTMLContentModel.mixed

[flaviadefaria]

@0xmiroslav have you had a chance to look at the updated proposal?

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[alitoshmatov]

@0xmiroslav So applying styles to wrapper element is not an option, right? Even if we apply it partially, say only fontSize and lineHeight?

[flaviadefaria]

@0xmiroslav are you able to choose a proposal?

[0xmiros]

We could change contentModel of h1 to HTMLContentModel.mixed to apply all styles for it self and add style marginTop for h1 tag to prevent default marginTop of h1

@dukenv0307 can you elaborate more detail where you will apply this change?

[0xmiros]

@0xmiroslav So applying styles to wrapper element is not an option, right? Even if we apply it partially, say only fontSize and lineHeight?

That might work but seems not ideal. We can revisit if no better solutions found

[0xmiros]

@0xmiroslav are you able to choose a proposal?

@flaviadefaria Still open for better proposals

[dukenv0307]

@0xmiroslav We will apply this change in customHTMLElementModels by create a new h1 tag that has contentModel is HTMLContentModel.mixed

[melvin-bot[bot]]

@amyevans @flaviadefaria @0xmiroslav this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[amyevans]

Brought discussion to Slack

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[amyevans]

It's been almost 4 weeks Melv 😛. Let's pick a path forward in the next 1-2 days @0xmiroslav

[mostfa29]

Proposal

Problem Statement

The issue is that when a link is added in header text, the App keeps the area above the link clickable but does nothing on click.

Root Cause

The problem is caused by the containerStyles prop in the Hoverable component which passes display: inline to the component. This causes the extra spacing above the link. https://github.com/Expensify/App/blob/615ebd98c4b32affe2875712187d202a7dc64ab0/src/components/Hoverable/index.js#L96

Proposed Solution

To solve the issue, we can get rid of the containerStyles prop and instead pass style={{display: 'inline-flex'}}. This will remove the extra spacing above the link. Additionally, it seems that this solution will not affect other components that inherit Hoverable.

style={{display: 'inline-flex'}}

Alternative Solutions (Optional)

Another alternative solution is to pass the style={{display: 'inline-flex'}} to the containerStyles prop in ReportActionItem.js.

<Hoverable containerStyles={{display: 'inline-flex'}}>

[dukenv0307]

@0xmiroslav I tested before, it's not good if change contentModel of h1 to mixed because it will disable boxStyle example is marginTop, marginBottom, ... So I have a new idea that is create new h1 tag that will be rendered is div to prevent default style of h1 in browser.

[0xmiros]

I will finalize review soon

[melvin-bot[bot]]

@amyevans @flaviadefaria @0xmiroslav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @0xmiroslav is eligible for the Internal assigner, not assigning anyone new.

[flaviadefaria]

@amyevans @0xmiroslav it seems like this was switched to internal does this mean that you both will be handling it?

[0xmiros]

There are still proposals to validate. I will finalize asap after handling urgent EC3 PRs

[flaviadefaria]

@0xmiroslav do you have an update here?