Open IuliiaHerets opened 1 month ago
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
IuliiaHerets
commented
1 month ago @puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
melvin-bot[bot]
commented
1 month ago Job added to Upwork: https://www.upwork.com/jobs/~021837227522028493087
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 (External)
huult
commented
1 month ago Edited by proposal-police: This proposal was edited at 2024-09-21 10:37:45 UTC.```
Android - Avatar - Profile avatar can be seen only if tap on blank screen
The layout is not changing, so onLayout is not being called, which prevents updateCanvasSize from being triggered.
https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Lightbox/index.tsx#L204-L207
So, the canvasSize will be undefined, and isCanvasLoading will be set to true.
https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Lightbox/index.tsx#L94-L95
isCanvasLoading is true, the image component will not render, resulting in a blank screen, and this issue occurs
https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Lightbox/index.tsx#L208
The reason onLayout is not being triggered is that shouldLoadAttachment is enabled after the parent component has rendered.
The reason onLayout is not being triggered is that the Send Button is rendered before the AttachmentView component. Its presence at the bottom might be affecting how the onLayout event is triggered. Since React Native handles layouts asynchronously, the positioning of components in the view hierarchy can influence when their dimensions are calculated and when onLayout is fired.
To solve this problem, we just need to render the Send Button at the same time as the AttachmentView. The code will change to something like this:
// .src/components/AttachmentModal.tsx#L580
- {!!onConfirm && !isConfirmButtonDisabled && (
+ {!!onConfirm && !isConfirmButtonDisabled && shouldLoadAttachment && (
<Button
ref={viewRef(submitRef)}
success
large
style={[styles.buttonConfirm, shouldUseNarrowLayout ? {} : styles.attachmentButtonBigScreen]}
textStyles={[styles.buttonConfirmText]}
text={translate('common.send')}
onPress={submitAndClose}
isDisabled={isConfirmButtonDisabled}
pressOnEnter
/>
)}[!Note] Note: As I tested, this issue only occurs on Android. We can either update it for Android only or for all platforms, and it will still work under this condition
What alternative solutions did you explore? (Optional)
We should set
We should set shouldLoadAttachment to true when isModalOpen is set. This way, the AttachmentView will render with the modal, allowing the onLayout for the attachment view to be triggered.shouldLoadAttachment to true when isModalOpen is set. This way, the AttachmentView will render same time with Send Button
// .src/components/AttachmentModal.tsx#L368
} else if (fileObject.uri) {
const inputModalType = getModalType(fileObject.uri, fileObject);
+ setShouldLoadAttachment(true);
setIsModalOpen(true);
setSourceState(fileObject.uri);
setFile(fileObject);
setModalType(inputModalType);
}We should use a ref so that the measurement occurs after the component has mounted, something like this:
// .src/components/Lightbox/index.tsx#L167
+ const viewRef = useRef<View>(null);
// .src/components/Lightbox/index.tsx#L172
+ useEffect(() => {
+ const measureView = () => {
+ viewRef.current?.measure((_, __, width, height) => {
+ setCanvasSize({
+ width: PixelRatio.roundToNearestPixel(width),
+ height: PixelRatio.roundToNearestPixel(height),
+ });
+ });
+ };
+ measureView();
+ }, []);
// .src/components/Lightbox/index.tsx#L204
<View
style={[StyleSheet.absoluteFill, style]}
onLayout={updateCanvasSize}
+ ref={viewRef}
>
struc
commented
1 month ago Avatar image does not show up or load.
On mobile, we use Lightbox to render the avatar image, however onLayout is never called because the view does not have a defined size yet.
Which also means the image view is never mounted and ImageView.onload is also never called.
https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/components/Lightbox/index.tsx#L204-L207
Initialize canvasSize with any value (i.e 0x0) will force a layout and mount the image view and thus load the image.
so we replace
with
const [canvasSize, setCanvasSize] = useState<CanvasSize>({width: 0, height: 0});N/A
https://github.com/user-attachments/assets/7928de0e-f53d-498c-99a1-84303a16ed58
dukenv0307
commented
1 month ago @huult @struc Thanks for your proposals. onLayout should be called on mount (works well in iOS). Can you dig deeper to find the RCA?
FYI, I just tested onLayout on snack.expo.dev, it worked well on Android
import React from 'react';
import {View, Text} from 'react-native';
const ViewBoxesWithColorAndText = () => {
const [a,setA] = React.useState(0)
return (
<View>
<Text>{a}</Text>
<View
onLayout={(e)=>{
setA(111111)
}}>
</View>
</View>
);
};
export default ViewBoxesWithColorAndText;The reason onLayout is not being triggered is that shouldLoadAttachment is enabled after the parent component has rendered.
@huult Can you elaborate that?
huult
commented
1 month ago Apologies, @dukenv0307 for the many changes in the proposal. Could you please review it again? I've updated the problem with more accuracy, and I appreciate your help. Thank you!
dukenv0307
commented
1 month ago @huult It doesn't really convince me, why does this happen on Android only?
melvin-bot[bot]
commented
1 month ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
huult
commented
1 month ago @dukenv0307 ,
I believe this issue is related to the rendering process in Android React Native. According to my RCA, the AttachmentView is rendered last, after all other components. I'm unsure why the onLayout is being prevented in AttachmentView.
With the solution I provided, rendering the Send Button at the same time as the AttachmentView seems reasonable and fixes the issue. I also tested it on the web and iOS, and it is still working well.
// .src/components/AttachmentModal.tsx#L580
- {!!onConfirm && !isConfirmButtonDisabled && (
+ {!!onConfirm && !isConfirmButtonDisabled && shouldLoadAttachment && (@puneetlath, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...
melvin-bot[bot]
commented
1 month ago @puneetlath, @dukenv0307 Still overdue 6 days?! Let's take care of this!
dukenv0307
commented
1 month ago I believe this issue is related to the rendering process in Android React Native
I tested this issue on React Native expo, I didn't face this issue. Can you dig deeper to find the exact RCA?
We also open to receive more proposals
melvin-bot[bot]
commented
1 month ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
melvin-bot[bot]
commented
1 month ago @puneetlath @dukenv0307 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!
puneetlath
commented
1 month ago @huult are you still interested in trying to find a solution for this?
huult
commented
1 month ago @puneetlath , Currently, I have found a workaround solution to fix this issue by rendering the Send Button at the same time as the AttachmentView. However, I have not yet found the exact root cause of why this issue only occurs on Android.
With the change below, I tested it and it's working as expected.
// .src/components/AttachmentModal.tsx#L580
- {!!onConfirm && !isConfirmButtonDisabled && (
+ {!!onConfirm && !isConfirmButtonDisabled && shouldLoadAttachment && (Ah ok I see. I think we should ideally try to figure out the root cause before proceeding with that solution.
huult
commented
1 month ago Yes, I've been debugging this many times, but I still haven't figured it out yet. =]] I'll post the exact RCA if I can find it
melvin-bot[bot]
commented
1 month ago @puneetlath, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
melvin-bot[bot]
commented
1 month ago @puneetlath, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?
dukenv0307
commented
1 month ago Waiting for more proposals
melvin-bot[bot]
commented
1 month ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
dukenv0307
commented
4 weeks ago Still waiting for proposals
melvin-bot[bot]
commented
4 weeks ago ⚠️ This issue has had its price increased by 4x or more. Please review the issue and ensure the price is correct.
melvin-bot[bot]
commented
4 weeks ago Upwork job price has been updated to $500
puneetlath
commented
4 weeks ago Increasing to $500 to see if we can get more proposals.
HezekielT
commented
3 weeks ago Android - Avatar - Profile avatar can be seen only if tap on blank screen
There are two issues.
View component is mainly used to layout their children, they will be automatically removed from the native hierarchy as an optimization unless we disable the optimization by making it non-collapsable as stated in the documenation.So the first issue is the View being removed on android.
View is not removed from the native hierarchy, the onLayout event is not fired properly due to animations as stated in the documentation resulting in updateCanvas not getting called on mount and in canvasSize being undefined and isCanvasLoading being true. The issue of onLayout not working properly due to animations on android is a known problem.In some (unknown, undocumented) situations onLayout is not called
Now, the reason why the avatar is shown when we tap on the blank screen is due to the fact that tapping the screen cause the component to re-render and given that onLayout is only triggered during on mount and on layout changes, the correct canvasSize will be calculated this time since canvasSize doesn't have the correct size(is undefined) and the avatar will be shown.
For the first issue, we need to add collapsable={false} to the view component, so that it will not be removed for optimization purpose.
For the second issue, we need to calculate the layout value manually when the component mounts using measure() method as shown below.
const viewRef = useRef<View>(null);
. . .
useEffect(() => {
if (viewRef.current) {
viewRef.current.measure((x: number, y: number, width: number, height: number) => {
setCanvasSize({
width: PixelRatio.roundToNearestPixel(width),
height: PixelRatio.roundToNearestPixel(height),
});
});
}
}, []);
. . .
<View
ref={viewRef}
onLayout={updateCanvas}
…. Alternatively, if we don’t want to use measure(), we can also initialize the canvasSize with {width: 0, height: 0} as this will also cause onLayout to be triggered again.
wildan-m
commented
3 weeks ago @dukenv0307, @IuliiaHerets Can you consistently reproduce it? I initially experienced it, but now it's not reproducible after retrying the steps.
Which Android device/version are you using? Did I missed something?
https://github.com/user-attachments/assets/bd5be308-7899-4a96-a138-51c72af4dce5
HezekielT
commented
3 weeks ago It can be reproduced reliably while sending an attachment.
Actual Result: Profile avatar can be seen only if tap on blank screen Same issue occurs when Take photo
wildan-m
commented
3 weeks ago It can be reproduced reliably while sending an attachment.
@HezekielT Thanks, it's more consistent that way
melvin-bot[bot]
commented
3 weeks ago @puneetlath @dukenv0307 this issue is now 4 weeks old, please consider:
Thanks!
melvin-bot[bot]
commented
3 weeks ago @puneetlath, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!
dukenv0307
commented
3 weeks ago I'm reviewing...
wildan-m
commented
3 weeks ago Edited by proposal-police: This proposal was edited at 2024-10-22 04:25:55 UTC.
Profile avatar can be seen only if tap on blank screen
The onLayout behavior differs between iOS and Android due to various factors. https://github.com/facebook/react-native/issues/28775#issuecomment-668831226 I believe isCanvasLoading is preventing the child component from being displayed, thus not triggering onLayout due to the lack of changes in the child component.
React Native Pager View faced a similar issue, which they addressed by manually refreshing the children layout in the Android code. https://github.com/callstack/react-native-pager-view/pull/351
Use PagerView instead of creating a new container for the single attachment view in src/components/AttachmentModal.tsx. Wrap AttachmentView with PagerView and disable scrolling to prevent image stretching.
Add inner view as described by the readme
Attention: Note that you can only use View components as children of PagerView https://github.com/callstack/react-native-pager-view?tab=readme-ov-file#usage
<AttachmentCarouselPagerContext.Provider value={context}>
<PagerView
useNext={shouldUseNewPager()}
style={[styles.imageModalImageCenterContainer]}
scrollEnabled={false}
>
<View
style={[styles.imageModalImageCenterContainer]}
>
<AttachmentView
containerStyles={[styles.mh5]}
source={sourceForAttachmentView}
isAuthTokenRequired={isAuthTokenRequiredState}
file={file}
onToggleKeyboard={setIsConfirmButtonDisabled}
onPDFLoadError={() => {
isPDFLoadError.current = true;
setIsModalOpen(false);
}}
isWorkspaceAvatar={isWorkspaceAvatar}
maybeIcon={maybeIcon}
fallbackSource={fallbackSource}
isUsedInAttachmentModal
transactionID={transaction?.transactionID}
isUploaded={!isEmptyObject(report)}
/>
</View>
</PagerView>
</AttachmentCarouselPagerContext.Provider>
Prevent isCanvasLoading from blocking the child component render in src/components/Lightbox/index.tsx
We've already provide logic to show loading indicator and show fallback image, we don't need to hide all child components We can disable MultiGestureCanvas if it's not loaded yet
change the render logic to:
<View
style={[StyleSheet.absoluteFill, style]}
onLayout={updateCanvasSize}
>
<>
{isLightboxVisible && (
<View style={[StyleUtils.getFullscreenCenteredContentStyles(), StyleUtils.getOpacityStyle(Number(shouldShowLightbox))]}>
<MultiGestureCanvas
isActive={isActive && !isCanvasLoading}
canvasSize={canvasSize}
canvas size will require initial value
const [canvasSize, setCanvasSize] = useState<CanvasSize>({width: 0, height: 0});
const isCanvasLoading = canvasSize.height === 0 && canvasSize.width === 0;
@wildan-m I faced the following issue when using your main solution
https://github.com/user-attachments/assets/885a1df0-8e47-46ab-b197-6e7349e310c6
Can we apply this fix on a new View instead of using PagerView
wildan-m
commented
3 weeks ago @dukenv0307 Ah, forgot this warning
Attention: Note that you can only use View components as children of PagerView https://github.com/callstack/react-native-pager-view?tab=readme-ov-file#usage
We should wrap inner element with View based on their guide
dukenv0307
commented
3 weeks ago @HezekielT Your solution seems like a workaround to me, we need to fix the problem here: the onlayout is not triggered.
I think the solution of @wildan-m looks promising since it's applied to other repos (such as https://github.com/callstack/react-native-pager-view).
dukenv0307
commented
3 weeks ago @wildan-m
Can we apply https://github.com/facebook/react-native/issues/17968#issuecomment-697136929 on a new View instead of using PagerView
Like this
HezekielT
commented
3 weeks ago @dukenv0307 The solution that was applied to react-native-pager-view is more or less the same to what i suggested since it uses measure(). Also the issues we are facing here is onlayout not being triggered on mount which my proposal aims to address it so i honestly don't think it's a workaround.
Btw, PagerView only works for ios and android which means we need separate files to maintain for web. do you think it's a good approach? instead of solving it directly using the measure() method
also the solution that's being suggested here is the same as to mine. i don't understand the problem here.
dukenv0307
commented
3 weeks ago @HezekielT
the issues we are facing here is onlayout not being triggered on mount
Yeah, we need to fix onlayout to be triggered on mount as what docs mentioned => calculating the layout value manually when the component mounts is the workaround to me
Btw, PagerView only works for ios and android which means we need separate files to maintain for web
Using PagerView is not the best solution for me. I suggested it in https://github.com/Expensify/App/issues/49553#issuecomment-2428222299
wildan-m
commented
3 weeks ago I've created PagerView wrapper for native specific code, non native will render it as standard View, so it won't error in non native
HezekielT
commented
3 weeks ago @dukenv0307
Yeah, we need to fix onlayout to be triggered on mount as what docs mentioned
could you please explain where you want the measure() to be applied?🙏 are you suggesting we apply it in react-native by patching it since the issue is with android's onlayout not working properly
dukenv0307
commented
3 weeks ago @HezekielT Yes, if it's react native issue, we should fix it in that lib
HezekielT
commented
3 weeks ago @dukenv0307 i'm really not sure if that's possible. The issue with onLayout not being correctly working on android has been a problem for a long time and a workaround has been the way to solving it. Could you please consider the workaround here as well since it's the issue is with how android handles layout🙏
Even the doc also indicate issues like this may arise
but the new layout may not yet be reflected on the screen at the time the event is received, especially if a layout animation is in progress.
melvin-bot[bot]
commented
3 weeks ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
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: 9.0.39-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
Profile avatar should be displayed when open it
Actual Result:
Profile avatar can be seen only if tap on blank screen Same issue occurs when Take photo
Workaround:
Unknown
Platforms:
Screenshots/Videos
https://github.com/user-attachments/assets/05367f4d-e64f-48cd-bf85-9ab211629dcb
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @s77rt