Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[$16000] Android/iOS - Code blocks are overflowing the app border #4733

Closed isagoico closed 8 months ago

isagoico commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue is failing https://github.com/Expensify/App/pull/4624 (CC @parasharrajat)

Action Performed:

  1. Navigate to a conversation in iOS or Android
  2. Send a long message in a code block

Expected Result:

Code block should be displayed in the area of the conversation.

Actual Result:

Code block is partially visible because is overflowing the app border.

Platform:

Where is this issue occurring?

Version Number: 1.0.86-2

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ecab5a591272dd78
  • Upwork Job ID: 1602379910299570176
  • Last Price Increase: 2022-12-27
MelvinBot commented 2 years ago

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

OSBotify commented 2 years ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
parasharrajat commented 2 years ago

@iwiznia even reverting https://github.com/Expensify/App/pull/4624 PR does not fix this issue. This issue is caused by upgrading the react-native-render-html. And I am tracking multiple issues caused by that here meliorence/react-native-render-html#514.

parasharrajat commented 2 years ago

I am investigating it.

iwiznia commented 2 years ago

Oh sorry about that, I think that this PR caused this then https://github.com/Expensify/App/pull/4047 cc @jsamr

iwiznia commented 2 years ago

Wait, I am confused, that PR was deployed a while ago, was this broken before and we did not notice?

parasharrajat commented 2 years ago

Yes. this is the list of issues that were caused by that upgrade.

  1. https://github.com/Expensify/App/issues/4488.
  2. https://github.com/Expensify/App/issues/4377 . (What is wrong with Github? URLs for this issue and link pasted have the same id) notice 4377.
  3. Links were clickable from empty space. https://github.com/meliorence/react-native-render-html/issues/514
  4. And this one.

It is just due to one reason that a wrapper node was removed in the new lib version for optimizations which I think should be reverted.

parasharrajat commented 2 years ago

Update, I have discussed this issue with the Lib owner and we are going to revert this change behind the flag which should fix the issue. It should be done by weekend.

isagoico commented 2 years ago

@johnmlee101 @parasharrajat Should this issue still be considered a deploy blocker?

Update: Just as a double check, I just retested this and it's still happening

parasharrajat commented 2 years ago

I think is happening on prod as well. so No deploy blocker.

Update:

  1. After applying the latest patch from the lib, this specific issue is not fixed. The library has moved a long way from the earlier version.
  2. I am still trying to find the best possible solution but currently, nothing seems to work.
  3. Next planned thing is to check the diff between older and newer versions and find the root cause. If the fix is patchable, I will approach the lib maintainers otherwise try to solve it with combinations of settings.
isagoico commented 2 years ago

Confirmed this is also happening in prod too in iOS. Removing the deploy blocker label.

parasharrajat commented 2 years ago

Update: This is really giving me nuts. I tried a lot of things but still no luck.

Planned:

I will revert the code to the old version before the upgrade of RNRH and try to observe why it was working. Hoping this will help.

johnmlee101 commented 2 years ago

Did you have a revert PR to link @parasharrajat ? Or any update here?

johnmlee101 commented 2 years ago

Still on hold for N6

johnmlee101 commented 2 years ago

No update, still on hold

parasharrajat commented 2 years ago

No update about the solution. But I ended with the conclusion that Using View inside the Text is not a good option Especially when you don't have control over the parent elements.

In this case, only words that extend beyond the width of the screen, will overflow.

At last, I will try to hack it but there are other priorities for me and this is yet on hold until N6.

MelvinBot commented 2 years ago

This issue has not been updated in over 15 days. @johnmlee101, @parasharrajat eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

johnmlee101 commented 2 years ago

Is this still reproducible?

parasharrajat commented 2 years ago

I think yes. But I haven't found any solution for it.

johnmlee101 commented 2 years ago

Any update?

parasharrajat commented 2 years ago

Oh. This is a long-dead issue. I completely forgot about this. Sorry, I have tried many solutions to adjust the UI but using only styling does not seem to be a solution anymore. I will try to find some tricks but truly saying existing implementation for code blocks rendering is already tricky. We can reopen this for others if needed, It will be some time before I can spend time on this again.

jsamr commented 2 years ago

I am currently studying in details differences between CSS spec and React Native layout. Perhaps I will publish some material at some point; this morning I have been working on CSS inline formatting contexts VS React Native Text layout.

The issue you are encountering is due to a lack of specification regarding layout of inline elements in React Native. Its layout engine, Yoga, only attempts compliance with CSS flexible layout, but doesn't claim any support for CSS normal flow, which determines, among other things, how runs of text mixed with other inline elements should be displayed. Therefore, the Text element creates an undocumented layout that resembles to some extent CSS inline layout. But its implementation is had-hoc and doesn't check against any kind of specification that I know of. For this very reason, it is also very prone to platform specifics. I have started collecting a few facts about behavioral divergence:

CSS inline-level element React Native (Text child of Text) React Native (View child of Text)
Inline margins are preserved.
Inline paddings are preserved.
Block margins are ignored.
Block paddings are preserved.
Block paddings/borders overflow the linebox*. n/a n/a
Borders are preserved.
Child text-runs are aligned to linebox baseline.
Child text-runs can be fragmented**.

* Unless for elements with an inline-block display type.
** They can be broken to participate to different lineboxes.

parasharrajat commented 2 years ago

Awesome, I will keep eye on your blog.

johnmlee101 commented 2 years ago

No update (?)

johnmlee101 commented 2 years ago

Same ^

johnmlee101 commented 2 years ago

None

johnmlee101 commented 1 year ago

Where are we with this?

johnmlee101 commented 1 year ago

Should I close this out?

parasharrajat commented 1 year ago

No, let's export this, Please add the external label. Hopefully, we can get some ideas. I will be auto-assigned as C+.

melvin-bot[bot] commented 1 year ago

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

parasharrajat commented 1 year ago

@bfitzexpensify was removed accidentally.

melvin-bot[bot] commented 1 year ago

@parasharrajat Eep! 4 days overdue now. Issues have feelings too...

parasharrajat commented 1 year ago

Bump @bfitzexpensify

bfitzexpensify commented 1 year ago

Ah, missed this because of the unassignment.

Upwork job is here

melvin-bot[bot] commented 1 year ago

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

Luke9389 commented 1 year ago

Ok @parasharrajat I just want to summarize the history of this issue to be sure I understand. This (along with a few other issues) was caused by an update here. You were investigating it but hit a dead end. And now you're the C+ here?

parasharrajat commented 1 year ago

Yup. you are correct.

Luke9389 commented 1 year ago

OK perfect! Thanks for the clarification 😄

bfitzexpensify commented 1 year ago

Doubled job to $500

bfitzexpensify commented 1 year ago

Doubled to $1000

bfitzexpensify commented 1 year ago

Doubled price to $2000.

bfitzexpensify commented 1 year ago

Doubled price to $4000

bfitzexpensify commented 1 year ago

Doubled again to $8000

swiftpipe commented 1 year ago

Proposal:

I found a small problem with InlineCodeBlock

Currently, line text in InlineCode is set fixed, however if length word is too large example aaaaaaaaaaaaaaaxxxxxzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz then when word wrap height is changed and so part of the text is hidden

I tried to remove the fixed height, and it worked

https://github.com/Expensify/App/blob/361736dce0bca7b1fbd861546fd417f0ef0cee9e/src/styles/codeStyles/index.ios.js#L1-L9

This current my code:

const codeWordWrapper = {
  //   height: 20, <--- comment this code
  justifyContent: 'center',
};

const codeWordStyle = {
  //   height: 16, <--- comment this code
  marginTop: 4, <--- top: 4 change to marginTop 4 to line separate
};

Result:

Before

Simulator Screen Shot - iPhone 13 - 2022-08-31 at 22 13 22 After

Simulator Screen Shot - iPhone 13 - 2022-08-31 at 22 37 23

I still don't understand the purpose of fixed height here. Please try again for me

parasharrajat commented 1 year ago

Thanks for the proposal. It's been some time on this issue. Let me go refresh my memory on it and then I will review the proposal. It could take a day or two.

I will do this

  1. which type of code blocks are we talking about? Inline or block?
  2. What is already tried in the past?
  3. Why do we have a few things in the code as the way they are?
melvin-bot[bot] commented 1 year ago

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

parasharrajat commented 1 year ago

Will do the review today.

parasharrajat commented 1 year ago

Expect a response tomorrow daytime IST.

swiftpipe commented 1 year ago

New Proposal

We will not modify the style of the InlineCodeBlock at all, because as I understand InlineCodeBlock the purpose of the words is on a single line and if it is too long, it will automatically break through a new line. However, if a word is too long, as I suggested before, it will be hidden because we had fixed the height of the block. So what do we do to solve it?

For words too long with a fixed length, I will change the logic for textMatrix

https://github.com/Expensify/App/blob/159249bb82317fcd6bd06bc8d4f9127c300a9f9c/src/components/InlineCodeBlock/WrappedText.js#L40-L49

This problem is textMatrix when the word is too long. If we find the word too long with a length greater than CONST.INLINE_BLOCK.MAX_CHARACTER, we split it up and just push back in the textMatrix array.

My solve In CONST.js INLINE_BLOCK: { MAX_CHARACTER: 100, },

/**
 * @param {String} text
 * @param {Number} fontSize
 * @returns {String[]}
 */
function splitText(text, fontSize) {
  if (!text) return [];
  const size = text.length * fontSize;
  if (size > CONST.INLINE_BLOCK.MAX_CHARACTER) return text.split('');
  return [text];
}

/**
 * @param {Array<String[]>} matrixText
 * @param {Number} fontSize
 * @returns {Array<String[]>}
 */
function transformMatrixText(matrixText, fontSize) {
  return matrixText.map((arrayText) => {
    let newArray = [];
    for (let i = 0; i < arrayText.length; i++) {
      const splitResult = splitText(arrayText[i], fontSize);
      newArray = [...newArray, ...splitResult];
    }
    return newArray;
  });
}
 ....
  const fontSize = lodashGet(props, 'textStyles.0.fontSize'); <-- I use fontSize to calculate width of word

  const textMatrix = getTextMatrix(props.children);

  const textMatrixTransformed = transformMatrixText(textMatrix, fontSize);

  return (
    <>
      {_.map(textMatrixTransformed, (rowText, rowIndex) => (

Result Before:

Simulator Screen Shot - iPhone 13 - 2022-09-08 at 09 56 15

After: Simulator Screen Shot - iPhone 13 - 2022-09-08 at 09 56 31

I think this to be pretty good because I tried it and saw how the markdown renderer looks like slack or github