[HOLD] [$250] [MEDIUM] Smartscan: Receipt thumbnail should show the top of the receipt

[m-natarajan]

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.4.22-6
Reproducible in staging?: yes
Reproducible in production?:.yes
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: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704757554079199

Action Performed:

1. create an IOU using receipt scanning
2. Observe the thumbnail of the receipt

Expected Result:

Should be showing from the top of the receipt

Actual Result:

Showing the middle portion of the receipt

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019bb6b442f89fba02
• Upwork Job ID: 1744585981661257728
• Last Price Increase: 2024-04-24
• Automatic offers:
• cubuspl42 | Reviewer | 28117185
• dukenv0307 | Contributor | 28117186

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019bb6b442f89fba02

[melvin-bot]

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

[melvin-bot]

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

[dukenv0307]

Proposal

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

Showing the middle portion of the receipt

What is the root cause of that problem?

For resizeMode cover, the image will have default positioning which is in the middle. The behavior that we want is similar to the object-position: top in CSS, but this React Native doesn't support it out of the box so we need to add it to React Native.

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

Make the View wrapping the image to have overflow hidden, for the image itself, it will take full width, variable height according to the aspectRatio (not fixed height).

The View wrapper already has overflow hidden here so all we have to do is to make sure our Image component can handle the object position top case.

1. Add a objectPositionTop boolean prop to our Image component to control this behavior (later we can modify to support other positions like bottom). I intended to make it as similar and reusable as possible (similar usage as native object-position: top in CSS), we just need to modify in common Image component and we can use it everywhere we use images in the app.
2. Add the prop to ImageWithSizeCalculation as well and pass it through to the inner Image, In here, add objectPositionTop because we want that behavior for ThumbnailImage. We can make it a prop of ThumbnailImage as well so we can control the position of ThumbnailImage.
3. Then handle in Image so the objectPositionTop property works
   Here

const [aspectRatio, setAspectRatio] = useState();

Here

RNImage.getSize(source.uri, (width, height) => {
    onLoad({nativeEvent: {width, height}});

    if (props.objectPositionTop) {
        setAspectRatio(height ? width / height : 'auto');
    }
});

Here

style={[forwardedProps.style, aspectRatio !== undefined && {aspectRatio, height: 'auto'}]}

For native platforms, we should do similarly in the index.native file, for native platforms, we won't get the image dimensions unless we load the image, we might want to use Image.getSize similar to on web to make sure it's independent from the image loading, and then we can prevent displaying of the image while the aspectRatio is not yet available, it might help prevent some potential issues.

We need to double check all possible cases of showing thumbnail receipt and add objectPositionTop to those places as needed.

What alternative solutions did you explore? (Optional)

Another approach is to translate the image vertically an amount such that instead of appearing from the center, image will appear at the top, this will require some math to calculate the translation so I prefer the aspectRatio approach above

I explored using object-position: top and background-position: top as well but react native does not yet support it.

[aswin-s]

Proposal

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

Receipt thumbnail is not showing the top of the image

What is the root cause of that problem?

For displaying a receipt <ThumbnailImage/> component is used with shouldDynamicallyResize prop set to false. This causes the image to fill to the given container width and height. However the ImageWithSizeCalculation component covers the image to available width and height. This ensures that the displayed image properly covers the parent container. However a side effect of cover is that the image gets scaled and positioned such that it entirely covers the container, there by causing the vertical or horizontal alignment of image to be adjusted as needed.

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

For always displaying the top section of receipt thumbnail, the image container width needs to be set to the width of thumbnail and the height should be set as per aspect ratio of the image. This makes sure that the thumbnail image gets rendered fully and not get scaled or repositioned due to 'cover'.

function ThumbnailImage({previewSourceURL, style, isAuthTokenRequired, imageWidth = 200, imageHeight = 200, shouldDynamicallyResize = true}: ThumbnailImageProps) {
    // Use a variable to save container width on layout
    const [containerSize, setContainerSize] = useState({ width: 0, height: 0 });
    ...

    // Now compute the aspect ratio of the image
    const aspectRatio = currentImageWidth && currentImageHeight ? currentImageWidth / currentImageHeight : 1;
    // Compute aspect ratio of container
    const containerAspectRatio = containerSize.width / containerSize.height;
    
    // Get static image size as per aspect ratios
    const getImageSize = useCallback(() => {
        if (!aspectRatio) {
            return [styles.w100, styles.h100]
        }
        if (aspectRatio > containerAspectRatio) {
            return [{
                height: containerSize.height,
                width: containerSize.height * aspectRatio
            }]
        }
        return [{
            width: containerSize.width,
            height: containerSize.width / aspectRatio
        }]
    }, [aspectRatio, containerAspectRatio, containerSize.height, containerSize.width, styles.h100, styles.w100])

    const sizeStyles = shouldDynamicallyResize ? [StyleUtils.getWidthAndHeightStyle(currentImageWidth ?? 0, currentImageHeight)] : getImageSize();
 
    return (
        <View
            style={[style, styles.overflowHidden]}
	    // Get container width             
            onLayout={(e) => setContainerSize(e.nativeEvent.layout)}
        >
            <View style={[...sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
                <ImageWithSizeCalculation
                    url={previewSourceURL}
                    onMeasure={updateImageSize}
                    isAuthTokenRequired={isAuthTokenRequired}
                />
            </View>
        </View>
    );

Above logic also handles images whose width is larger than height. Also the logic works for both native and web platforms.

There are a couple of screens like MoneyRequestConfirmationList and ReportActionItemImage etc where Image component is used instead of ThumbnailImage for displaying receipt. Such instances need to be replaced so that all occurrences of receipt image is properly displayed

Result

iOS

fix-ios_out.mp4

Web

fix-web_out.mp4

What alternative solutions did you explore? (Optional)

None (edited)

[dukenv0307]

@aswin-s FYI your solution's the same as one of my alternate solution

A potential alternative is to make the View wrapping the image to have overflow hidden, for the image itself, it will take full width, variable height according to the aspectRatio (not fixed height).

I'm writing the implementation details for my proposal and updating soon

[shubham1206agra]

Proposal

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

Receipt thumbnail is not showing the top of the image

What is the root cause of that problem?

Inside ReportActionItemImage, the ThumbnailImage component is being used. But this component uses 100% for height and width, which makes image center (looks like center but not exactly).

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

We need to use aspectRatio technique to properly scale the images so that always top can be shown. We need to correctly scale the images for landscape, and portrait images.

Test branch - https://github.com/shubham1206agra/App/tree/test-thumbnail

Note - Naming of function is not final.

What alternative solutions did you explore? (Optional)

NA

[aswin-s]

Noted @dukenv0307. My proposal has just one approach detailed which works on all platforms. I've tested on both iOS and web and shared the screen grabs as well. I've also addressed scenarios like serving local image when receipt upload is in progress in the above video. So it is not exactly same or abstract like your proposal.

I'll leave the merit of the proposal to the C+.

[shubham1206agra]

Noted @dukenv0307. My proposal has just one approach detailed which works on all platforms. I've tested on both iOS and web and shared the screen grabs as well. I've also addressed scenarios like serving local image when receipt upload is in progress in the above video. So it is not exactly same or abstract like your proposal.

I'll leave the merit of the proposal to the C+.

Don't worry. C+ will take care of these things :)

[cubuspl42]

@dukenv0307

I'll first ask about your main solution.

object-position: top

I assume you mean the object-position CSS property. How would this solve the problem on all supported platforms?

@aswin-s @shubham1206agra

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

@shubham1206agra

Are landscape and portrait some code / CSS constants?

Do I understand correctly that you, like @aswin-s, suggest adjusting the math of ThumbnailImage, but in a different way?

[shubham1206agra]

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

I have a prop to control that shouldShowTop, if it is false, then it will revert to older configuration (100% one).

Are landscape and portrait some code?

No, its image orientation.

Do I understand correctly that you, like @aswin-s, suggest adjusting the math of ThumbnailImage, but in a different way?

Yes

[dukenv0307]

Proposal updated to include implementation details.

I assume you mean the object-position CSS property. How would this solve the problem on all supported platforms?

@cubuspl42 I tested and it doesn't seem to work on all supported platforms so I updated my proposal with a custom prop so it can be reusable across all images component, just like object-position: top.

FYI my solution is a generic one that will work for all occurences of Image, it's not relying on the ThumbnailImage implementation

[aswin-s]

@aswin-s @shubham1206agra

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

@cubuspl42 Yes my proposal is to modify the logic in ThumbnailImage component so that the image aligns to top when shouldDynamicallyResize is set to false.

Thumbnail component is used currently for Image attachment previews and Receipt previews. For image attachments in ReportAction, we always show the entire image in the thumbnail and shouldDynamicallyResize is always set to true. For receipt thumbnails the prop shouldDynamicallyResize is always set to false so that the thumbnail size is always fixed irrespective of the receipt image size.

In my proposal image alignment is changed only when shouldDynamicallyResize is set to false. So image attachment thumbnails in chat will remain unaffected. There is not enough differences in implementation to warrant for a completely different component like ReceiptThumbnail.

[cubuspl42]

@shubham1206agra Thank you. Let's use Markdown code syntax for technical and code identifiers only, as it can create confusion otherwise.

@aswin-s You accidentally double-sent your comment

[cubuspl42]

@aswin-s Your analysis of the shouldDynamicallyResize usage was helpful and important in the context of this issue, thanks. If shouldDynamicallyResize = false is used for and only for receipt previews, we can just modify this code path in the YAGNI spirit.

@shubham1206agra The code branch is, in my opinion, a helpful tool for sharing proof of basic testing and specifics of the proposal. But, like on StackOverflow: links are considered helpful and desired additions, but link-only answers are against the guidelines. So far, your solution seems (especially without looking at the code diff) very similar to the one by @aswin-s. Unless you handle some critical and objective case(s) that the other solution doesn't, I don't think it is a substantial improvement.

[dukenv0307]

@cubuspl42 what do you think about my proposal?

I don't think @aswin-s 's ThumbnailImage solution will work for receipts that don't use ThumbnailImage to display (like local images). And we shouldn't replace those with ThumbnailImage because ThumbnailImage has a lot of overhead, and we don't need such replacement to fix this issue.

[cubuspl42]

@dukenv0307 I'm confused as to why you started with a pure CSS solution and switched to a cross-platform one late. I haven't yet analyzed the updated solution.

ThumbnailImage solution will work for receipts that don't use ThumbnailImage to display (like local images)

This is an important observation. I missed this fact. Indeed, there's some conditional logic in ReportActionItemImage.

When is the receipt a local image? How does alignment work now in such a case?

[aswin-s]

@cubuspl42 I've mentioned about handling other receipt images in my proposal.

There are a couple of screens like MoneyRequestConfirmationList and ReportActionItemImage etc where Image component is used instead of ThumbnailImage for displaying receipt. Such instances need to be replaced so that all occurrences of receipt image is properly displayed

I also mentioned about it in my response to dukenv0307

I've also addressed scenarios like serving local image when receipt upload is in progress in the above video. So it is not exactly same or abstract like your proposal.

When the receipt image is getting uploaded it shows image url from local device or from blob (web platform). It gets handled over here.

App/src/components/ReportActionItem/ReportActionItemImage.js

Lines 79 to 82 in a4bdebe

	<Image
	source={{uri: thumbnail || image}}
	style={[styles.w100, styles.h100]}
	/>

And for confirmation page over here.

App/src/components/MoneyRequestConfirmationList.js

Lines 607 to 616 in a4bdebe

	{(receiptImage || receiptThumbnail) && (
	<Image
	style={styles.moneyRequestImage}
	source={{uri: receiptThumbnail || receiptImage}}
	// AuthToken is required when retrieving the image from the server
	// but we don't need it to load the blob:// or file:// image when starting a money request / split bill
	// So if we have a thumbnail, it means we're retrieving the image from the server
	isAuthTokenRequired={!_.isEmpty(receiptThumbnail)}
	/>
	)}

Both these places use Image component to show the thumbnail at the moment. So if we want the same behaviour as in chat, these should be replaced with ThumbnailImage component. That's what I mentioned in my proposal towards last.

Also the iOS video recording shows this scenario handled on all screens.

[dukenv0307]

@dukenv0307 I'm confused as to why you started with a pure CSS solution and switched to a cross-platform one late

@cubuspl42 as mentioned here, my alternate solution in the initial proposal is the aspectRatio approach (similar to what others proposed later above). At the time you reviewed I was still trying to get the object-position solution working because if possible it will be the cleanest, but I couldn't find any since RN doesn't support it.

So I updated my proposal in here with the implementation details of my initial aspectRatio solution.

aswin-s: Both these places use Image component to show the thumbnail at the moment. So if we want the same behaviour as in chat, these should be replaced with ThumbnailImage component. That's what I mentioned in my proposal towards last.

@cubuspl42 So with this the logic is now coupled with the ThumbnailImage component and anywhere we use receipt we now have to convert to ThumbnailImage, which has unnecessary overhead compared to the Image component in those cases.

With my proposal the the objectPositionTop logic is contained within the Image component and will work well for all those cases (no need for all-to-ThumbnailImage refactor).

So the issue has nothing to do with ThumbnailImage component (it's not the root cause), it's just how resizeMode: cover behaves for Image in react-native, thus we should fix it there at the root.

[DanutGavrus]

Proposal

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

Receipt thumbnail should show the top of the receipt.

What is the root cause of that problem?

We are using width: 100% and height: 100% in combination with resizeMode: cover which crops the image to the middle, both vertically and horizontally.

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

Smallest change to make in order to fix this issue is to replace height: 100% with height: *actual height of the thumbnail* in ReportActionItemImage for the ThumbnailImage component. The overflow is hidden, and the image will automatically start from the top with no other changes. If we want to make this change in other places too, we can generalise this approach. In order to obtain the actual height of the image in ReportActionItemImage we may use: RNImage.getSize(`${thumbnail}?authToken=${NetworkStore.getAuthToken()}`, (width, height) => {}) where RNImage is imported directly from react-native like import {Image as RNImage} from 'react-native'.

[cubuspl42]

C+ reviewed 🎀 👀 🎀

I approve the proposal by @aswin-s

The idea of this solution was (barely) mentioned before in the proposal by @dukenv0307. What is being reviewed, though, is a proposal as a whole. The main proposed solution, at the time of my initial review, was fundamentally HTML-specific (it mentioned div tags and pure CSS properties), so this is a big minus for that one in my opinion.

Also, @dukenv0307 disagrees with the mentioned idea, so I wouldn't like to force them to implement it:

And we shouldn't replace those with ThumbnailImage because ThumbnailImage has a lot of overhead

(the claim wasn't backed by measurements or any other source, though)

The proposal by @aswin-s contained solely the ThumbnailImage aspect ratio solution, going into a reasonable level of detail.

Thanks, everyone! As is often the case, other proposals also contained solutions that could work. Remember that the order in which the proposals are posted is not the main selection criterium.

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $500

[mallenexpensify]

@Santhosh-Sellavel reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks
@dukenv0307 , can you please update @Santhosh-Sellavel on where we're at here?

[dukenv0307]

@Santhosh-Sellavel We're in a follow-up PR here #40763 to also fix the bug for the confirmation page as well.

[Beamanator]

@dukenv0307 it looks like we've been showing the top of receipts for distance request receipts as well, but we don't want that behavior - see #40791

Also most likely I'll have to adjust my backend fix to only send receipt thumbnails focused on the top center IFF the receipt is NOT a distance request 🤔

[melvin-bot]

Upwork job price has been updated to $250

[garrettmknight]

@adelekennedy dropping to $250 for a regression.

[melvin-bot]

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

[Santhosh-Sellavel]

@mallenexpensify/@ad Can you assign me here please, seems I got unassigned accidentally

@Santhosh-Sellavel reassigning, please take over as C+. If you don't have bandwidth, unassign yourself.

[mallenexpensify]

Done, added @Santhosh-Sellavel , sorry about that

[melvin-bot]

@Beamanator, @adelekennedy, @Santhosh-Sellavel, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@Beamanator, @adelekennedy, @Santhosh-Sellavel, @dukenv0307 Still overdue 6 days?! Let's take care of this!

[Beamanator]

Hmm @dukenv0307 your PR went to prod a week or so ago, are we waiting on anything here?

[dukenv0307]

@Beamanator We're waiting for payment here.

[mallenexpensify]

Looks like this hit production on 4/23, so gonna pay
#35041 (comment)

Contributor: @dukenv0307 paid $250 via Upwork
Contributor+: @Santhosh-Sellavel owed $250 via NewDot