[$500] Chat - Scroll bar appears in the suggestion list when only one emoji is in the suggestion list

[lanitochka17]

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

---

Version Number: 1.3.96-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go to any chat
3. Type :+1

Expected Result:

There is no scroll bar on the right of the list since there is only one emoji in the suggestion list

Actual Result:

There is a scroll bar on the right of the list when only one emoji is in the suggestion list

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6267321_1699361414740.bandicam_2023-11-07_20-38-23-117.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018b10f4eb010b9828
• Upwork Job ID: 1721906785220390912
• Last Price Increase: 2023-11-07

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018b10f4eb010b9828

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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
• 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.
• 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.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[bernhardoj]

Proposal

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

Scrollbar shows even when the emoji suggestion is only one (it's also scrollable).

What is the root cause of that problem?

The list is scrollable because the View that wraps the list has a smaller height.

App/src/components/AutoCompleteSuggestions/BaseAutoCompleteSuggestions.js

Lines 90 to 95 in cbfd424

	<Animated.View
	ref={props.forwardedRef}
	style={[styles.autoCompleteSuggestionsContainer, animatedStyles]}
	exiting={FadeOutDown.duration(100).easing(Easing.inOut(Easing.ease))}
	>
	<FlatList

The height style comes from the animatedStyles and the value is from StyleUtils.getAutoCompleteSuggestionContainerStyle.

Each emoji suggestion has a height of 40. The suggestion component has a vertical padding of 8.

If we look at the implementation of getAutoCompleteSuggestionContainerStyle, we can see that the author already considers the amount of vertical padding.

App/src/styles/StyleUtils.ts

Lines 997 to 1010 in cbfd424

	function getAutoCompleteSuggestionContainerStyle(itemsHeight: number): ViewStyle {
	'worklet';
	const borderWidth = 2;
	const height = itemsHeight + 2 * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING;
	// The suggester is positioned absolutely within the component that includes the input and RecipientLocalTime view (for non-expanded mode only). To position it correctly,
	// we need to shift it by the suggester's height plus its padding and, if applicable, the height of the RecipientLocalTime view.
	return {
	overflow: 'hidden',
	top: -(height + CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_PADDING + borderWidth),
	height,
	};
	}

So, the total height would be 56. However, they seem to forget that the suggestion component also has a border (or maybe the border is added recently?).

The suggestion border width is 1, so we should add 2 * 1 to the height and the correct total height would be 58.

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

Add the border width to the calculation of the suggestion view height.

const borderWidth = 2;

-const height = itemsHeight + 2 * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING;
+const height = itemsHeight + 2 * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING + borderWidth;

[tienifr]

Proposal

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

Chat - Scroll bar appears in the suggestion list when only one emoji is in the suggestion list

What is the root cause of that problem?

We calculate the height for suggestion container but do not include the border width

App/src/styles/StyleUtils.ts

Line 1001 in cbfd424

const height = itemsHeight + 2 * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING;

-> The height of container is smaller than the inner one, that cause the scroll bar appears

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

Add borderWidth to height

    const height = itemsHeight + 2 * CONST.AUTO_COMPLETE_SUGGESTER.SUGGESTER_INNER_PADDING + borderWidth;

Result

[DrLoopFall]

Proposal

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

Chat - Scroll bar appears in the suggestion list when only one emoji is in the suggestion list

What is the root cause of that problem?

We are calculating rowHeight inside useEffect with withTiming, so when accessing the value for showsVerticalScrollIndicator we are actually using the old value.

App/src/components/AutoCompleteSuggestions/BaseAutoCompleteSuggestions.js

Lines 75 to 76 in 5d45f0f

	useEffect(() => {
	rowHeight.value = withTiming(measureHeightOfSuggestionRows(props.suggestions.length, props.isSuggestionPickerLarge), {

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

Move measureHeightOfSuggestionRows outside the useEffect hook so that the calculated height would be in sync.

+ const actualHeight = measureHeightOfSuggestionRows(props.suggestions.length, props.isSuggestionPickerLarge);
  useEffect(() => {
-   rowHeight.value = withTiming(measureHeightOfSuggestionRows(props.suggestions.length, props.isSuggestionPickerLarge), {
+   rowHeight.value = withTiming(actualHeight, {

  // in <FlatList
-     showsVerticalScrollIndicator={innerHeight > rowHeight.value}
+     showsVerticalScrollIndicator={innerHeight > actualHeight}

---

If wanted we can also update the height here, but only updating the height would still show the scrollbar while animating, so we need to use the above solution.

[garrettmknight]

Unable to reproduce on v1.3.96-15 via chrome or v1.3.95-9 MacOS. Closing - @lanitochka17 please reopen if you can get it to show.

[DrLoopFall]

Hi @garrettmknight,
Can you please check again, this issue is very much reproducible in v1.3.95-9 and v1.3.97-1.
In MacOS the scrollbar is floating(hidden when not scrolling) by default when using the trackpad so you have to scroll on the suggestion list, to see the scrollbar, please check the screenshot below.

MacOS:

In other platforms where the scrollbar is non-floating by default, you can see it without scrolling.

[DrLoopFall]

@garrettmknight, Also in MacOS when using a mouse instead of the trackpad the scrollbar will be visible by default, so it will be visible even without scrolling.

MacOS with mouse:

[DrLoopFall]

Hi @garrettmknight @lanitochka17,
Just a friendly bump, please have a look at this.

[lanitochka17]

I am able to reproduce an issue in Windows10/Chrome
Build 1.3.97-7

[brunovjk]

I was also able to reproduce ithe issue.

@bernhardoj I can reproduce your solution, it solves the problem, however it seems that the emoji and the suggestion are not vertically aligned, the emoji is not in the center I guess. Does it make sense or is it normal behavior? Thanks.

[derekops333]

Proposal

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

Redundant scroll bar appears in the suggestion list when only one emoji is in the list

What is the root cause of that problem?

In the <BaseAutoCompleteSuggestions /> component, the showsVerticalScrollIndicator prop of the <FlatList /> is always set to true. It is the result of the innerHeight and rowHeight.value comparison. Unfortunately, nothing triggers component re-render once the rowHeight.value is calculated (since it is a ref, not the state). Because of that, component keeps it's initial look where the scroll bar is visible.

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

Even though we still need to keep rowHeight as a reference, as it is right now, we need to introduce a new state:
const [rowHeightValue, setRowHeightValue] = useState(0);

in the useEffect hook, where we call withTiming function, we should set a third parameter of the withTiming function - a callback in which we will set the new state value, once the calculation is finished.
Once the state is updated, a component re-render is triggered, and we can use a new state value for the showsVerticalScrollIndicator comparison

Solution Video:
Screencast from 11.11.2023. 23:52:47.webm

[melvin-bot]

📣 @derekops333! 📣
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. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. 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>

[derekops333]

Contributor details
Your Expensify account email: derekops333@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~012e0374342e1c1946

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

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

[robertKozik]

Sorry for the delay on my end. I'll go through the proposals asap 👀

[robertKozik]

Hi all! Once more sorry for that delay on this issue 🙇🏼 After checking all proposals I believe we should go with the solution from @bernhardoj proposal. It offers easy to implement fix which covers the issue. It's also the first proposal on github issue timeline which suggests this approach (edit of the proposal is just cosmetic, it adds the code diff section only)
I want also to address other proposals which I considered, and give proper explanation on my decision:
@DrLoopFall - I've tried your solution based on proposal, what I've experienced was that the container was still scrollable - just without the visible indicator. Also perfectly I would avoid hoisting the calculations to the component function due to performance reasons
@derekops333 - I prefer to avoid introducing another state variable while we have other viable solutions without it

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[DrLoopFall]

Hi @robertKozik,

I've tried @bernhardoj's proposal, it'll cause a regression, it makes the scrollbar visible while the suggestion list is animating, check the video below, where the scrollbar flashes for a quick second.

2023-11-14.02-37-34.mp4

Regarding your concerns,

I've experienced was that the container was still scrollable

I've also mentioned about fixing the height in my proposal, with an explanation of why just fixing the height won't solve the issue.

From my proposal

If wanted we can also update the height here, but only updating the height would still show the scrollbar while animating, so we need to use the above solution.

---

Also perfectly I would avoid hoisting the calculations to the component function due to performance reasons

The measureHeightOfSuggestionRows only consists of a few if's and *'s, this won't cause any slowness, can you please check the implementation of measureHeightOfSuggestionRows.

[DrLoopFall]

@robertKozik
As, I already said in my comments (here and here), if you're using MacOS without a mouse, the above-mentioned regression can be difficult to spot, it would be visible to all the users using other platforms or MacOS with a mouse.

To test it, you may try enabling the scrollbar while using a MacOS with a trackpad.

[bernhardoj]

I think it's technically make sense that the scrollbar is shown when the list height is animating. The animation starts from a small height that of course will make the content scrollable. At the end, we need to fix the wrong calculation of the height.

[DrLoopFall]

@bernhardoj,
It actually doesn't make any sense, why would anyone need a scrollbar to be shown when the list height is animating, also it looks awfully worse and buggy when we have more items in the list, check this recording.

2023-11-14.09-45-29.mp4

[DrLoopFall]

Hi @robertKozik,

I've experienced was that the container was still scrollable

The issue you mentioned above was not because of my solution, it was already present, if we fix the height it'll cause issue #18112 again because it looks like issue #18112 was fixed based on the extra scroll height, you can check this video which has the extra scroll height from PR #19621 which fixes the issue.

Also, implementing @bernhardoj's proposal will make the issue #18112 reappear again.

[robertKozik]

Thanks for linking the issue, as it sheds a new light on our issue. Let me get more familiar with it first and I'lll get back here soon 😉

I agree that your changes would marginally affect the performance. What I meant when writing the reasoning why I prefered other proposal over yours @DrLoopFall was that ideally I opt for omitting recalculating any value every render, if thats possible of course

[derekops333]

Thanks for linking the issue, as it sheds a new light on our issue. Let me get more familiar with it first and I'lll get back here soon 😉

I agree that your changes would marginally affect the performance. What I meant when writing the reasoning why I prefered other proposal over yours @DrLoopFall was that ideally I opt for omitting recalculating any value every render, if thats possible of course

Solution that was proposed by the @DrLoopFall is a common React anti-pattern. It calls an expensive calculation inside a render function which blocks rendering of the component content and decreases the app performances.
My solution is the only one that calls expensive measureHeightOfSuggestionRows function the minimum amount of time, does not block content rendering and takes care that the component is re-rendered only once we have a new data (height) available

[DrLoopFall]

@derekops333,
Just have a look at this comment.

The measureHeightOfSuggestionRows only consists of a few if's and *'s, this won't cause any slowness, can you please check the implementation of measureHeightOfSuggestionRows.

For your reference, this is how measureHeightOfSuggestionRows is implemented.

const measureHeightOfSuggestionRows = (numRows, isSuggestionPickerLarge) => {
    if (isSuggestionPickerLarge) {
        if (numRows > CONST_VARIABLE) {
            return CONST_VARIABLE * CONST_VARIABLE;
        }
        return numRows * CONST_VARIABLE;
    }
    if (numRows > 2) {
        return CONST_VARIABLE * CONST_VARIABLE;
    }
    return numRows * CONST_VARIABLE;
};

Do you even realize that your proposed solution is the most expensive among all the proposals here, because you literally proposed using useState, which would require additional rerenders running hundreds of lines of codes, making it at least a thousand times more expensive than calling a simple function.

Do you even have any idea, how much expensive is to rerender?

[derekops333]

FlatList component (inside of the BaseAutoCompleteSuggestions) receives a prop showsVerticalScrollIndicator={innerHeight > rowHeight.value} (at the moment). It requires re-render each time the row height is changed. My solution will cause re-rendering only when it is needed - when we have a new row height calculated. If you put measureHeightOfSuggestionRows inside the render function, you will need to keep it outside of withTiming which then changes the already existing behavior and removes the animation effect

[DrLoopFall]

@derekops333,
Please study the codebase thoroughly, currently the height is animated with react-native-reanimated, which won't rerender for the value changes, it directly animates the view, if you want you can check by a simple console.log in BaseAutoCompleteSuggestions component.

Currently, the component BaseAutoCompleteSuggestions is rendered only once to show the suggestion list, but your solution makes it render twice.

If you put measureHeightOfSuggestionRows inside the render function, you will need to keep it outside of withTiming which then changes the already existing behavior and removes the animation effect

In my proposed solution the animation is working fine, check my proposal again for more details.

[lakchote]

@robertKozik, after considering @DrLoopFall's comments about a possible regression, what do you think about the proposals, especially the one you initially chose?

[melvin-bot]

@garrettmknight @lakchote @robertKozik this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[0xmiros]

This is dupe of #27866
Context: #27866 (comment)

[0xmiros]

Note: no one provided correct solution to avoid #27140.
That being said, we can close this one and reopen #27866 which had more context and was closed as minor.

[robertKozik]

Thanks for pointing it out @0xmiroslav if this is a dupe, I think we should close this issue then as suggested above

[garrettmknight]

Cool, closing!