melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~018e2392a94caf511b
Closed kavimuru closed 1 year ago
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~018e2392a94caf511b
melvin-bot[bot]
commented
1 year ago Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @MariaHCD (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
alexxxwork
commented
1 year ago I see it on Windows / Chrome too
PDF Box can not upload user asset{"jsonCode":502,"message":"502 Internal Error - Failed to run convert \/tmp\/5574e8d4202468e466f3822d22136f477a31e938 1, [\"convert: no decode delegate for this image format `' @ error\\\/constitute.c\\\/ReadImage\\\/560.\",\"convert: no images defined `\\\/tmp\\\/5574e8d4202468e466f3822d22136f477a31e938.jpeg' @ error\\\/convert.c\\\/ConvertImageCommand\\\/3258.\"]"}There's an edge case when we are in the process of editing our avatar in profile settings. If you start to resize the browser window you may get a situation when avatar image goes away. This leads to console errors and empty image as a result. Also you can get negative avatar dimentions which is obviously not good.
The problem is that on GestureHandlerRootView resize width and height changes but avatar image offset don't. So we should update offset to meet the new size of GestureHandlerRootView. https://github.com/Expensify/App/blob/5c1e2c55192a5071b54cf5f0fd974643eef73c25/src/components/AvatarCropModal/AvatarCropModal.js#L85
Also, the dimensions of image view are calculated through another component and there's another problem when the view size is smaller then styles.imageCropRotateButton.height that leads to imageContainerSize < 0 and console errors about wrong avatar mask SVG size.
We should alter the code where we calculate underlying avatar image position like so:
setImageContainerSize(Math.floor(Math.min(Math.max(height - styles.imageCropRotateButton.height, CONST.AVATAR_CROP_MODAL.INITIAL_SIZE), width)));
updateImageOffset(translateX.value,translateY.value);We get width and heigth of the avatar image editor indirectly from another component dimensions:
https://github.com/Expensify/App/blob/5c1e2c55192a5071b54cf5f0fd974643eef73c25/src/components/AvatarCropModal/AvatarCropModal.js#L320
We pass an onLayout handler to a GestureHandlerRootView component and we get width an height of the layout in this handler
https://github.com/Expensify/App/blob/5c1e2c55192a5071b54cf5f0fd974643eef73c25/src/components/AvatarCropModal/AvatarCropModal.js#L82
We can't use this handler in ImageCropView because it is displayed conditionally.
As you can see on the screenshot
we have controls in the bottom of GestureHandlerRootView so we need to substract the controls height from the layout height.
And here's the problem because we can resize browser window so that controls height could be greater than layout height.
Another problem is that we should update the avatar image position on layout update (if thinks it's just the right place to do it) so that we can't get out of the borders of the avatar image so we should call updateImageOffset(translateX.value,translateY.value); for that. This call is safe because it just updates shared values translateX, translateY based on GestureHandlerRootView dimensions.
oesayan
commented
1 year ago I honestly think we also have to do a post trimming check - is the data we send a correct picture?
parasharrajat
commented
1 year ago Not sure I understand. Can you guys add more details? @alexxxwork I don't see any solution being proposed. What do you mean by RCA?
alexxxwork
commented
1 year ago Not sure I understand. Can you guys add more details? @alexxxwork I don't see any solution being proposed. What do you mean by
RCA?
Should I first post a root cause analysis or it's not the case?
diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index 1d9922ead4..9928928ba9 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -82,7 +82,8 @@ const AvatarCropModal = (props) => {
const initializeImageContainer = useCallback((event) => {
setIsImageContainerInitialized(true);
const {height, width} = event.nativeEvent.layout;
- setImageContainerSize(Math.floor(Math.min(height - styles.imageCropRotateButton.height, width)));
+ setImageContainerSize(Math.floor(Math.min(Math.max(height - styles.imageCropRotateButton.height, CONST.AVATAR_CROP_MODAL.INITIAL_SIZE), width)));
+ updateImageOffset(translateX.value,translateY.value);
}, [props.isSmallScreenWidth]);
// An onLayout callback, that initializes the slider container size, for proper render of a slider
maciejmatu
commented
1 year ago When avatar container changes width due to window resizing the image px translation value is not being updated accordingly which causes the scaled image to be moved outside of actual bounds and results in sending malformed cropped image.
Modify src/components/AvatarCropModal/AvatarCropModal.js to run similar logic as in panSliderGestureEventHandler when sliderContainerSize is changed.
// This effect is needed to prevent the scaling of the image
// when the window's layout changes
useEffect(() => {
const newSliderValue = clamp(translateSlider.value, [0, sliderContainerSize]);
const newScale = newScaleValue(newSliderValue, sliderContainerSize);
const differential = newScale / scale.value;
scale.value = newScale;
translateSlider.value = newSliderValue;
const newX = translateX.value * differential;
const newY = translateY.value * differential;
updateImageOffset(newX, newY);
}, [sliderContainerSize]);To make it a bit nicer, we can separate logic to a single function that would be used in both of those places. There is still one edge case I have to cover, but it only happens when the image is so small that it's impossible to actually modify it.
Prince-Mendiratta
commented
1 year ago The root cause behind this error is that as the window is resized, the updated containerSize resizes the image. However, the offsets of the image are not updated respective to the updated width and height of the window. The only way currently to update the relative offsets is through the panGestureEventHandler handler. Thus, even when the image is displayed out of bounds in the small screen, it gets resized properly when the image is clicked even in the zoomed state.
Now, the reason why this is an issue is because that once the offsets are out of bounds, the originX and originY exceed the size of the image. This leads to the case where the cropped area of the image contains no image and thus, the error.
The best case scenario would be to ensure that the offsets are calculated properly on each window resize. This can be achieved by somehow triggering the offset re calculation in the initializeImageContainer function, since this is triggered on window resize. The issue however is that the calculation of the relative image offsets require the context, as shown in this snippet -
https://github.com/Expensify/App/blob/5c1e2c55192a5071b54cf5f0fd974643eef73c25/src/components/AvatarCropModal/AvatarCropModal.js#L194-L199
In addition to the solution 1, we should also add the limits to the allowed offset values. This will make sure that no matter what kind of image resizing bugs occur, other than a visual glitch, the functionality remains unaffected.
diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index 1d9922ead4..a1c98784a1 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -260,11 +261,13 @@ const AvatarCropModal = (props) => {
const imageCenterX = originalImageWidth.value / 2;
const imageCenterY = originalImageHeight.value / 2;
const apothem = size / 2; // apothem for squares is equals to half of it size
+ const Hmax = originalImageHeight.value - size;
+ const Wmax = originalImageWidth.value - size;
// Since the translate value is only a distance from the image center, we are able to calculate
// the originX and the originY - start coordinates of cropping view.
- const originX = imageCenterX - apothem - ((translateX.value / imageContainerSize / scale.value) * smallerSize);
- const originY = imageCenterY - apothem - ((translateY.value / imageContainerSize / scale.value) * smallerSize);
+ const originX = clamp((imageCenterX - apothem - ((translateX.value / imageContainerSize / scale.value) * smallerSize)), [-Wmax, Wmax]);
+ const originY = clamp((imageCenterY - apothem - ((translateY.value / imageContainerSize / scale.value) * smallerSize)), [-Hmax, Hmax]);
const crop = {
height: size, width: size, originX, originY,In addition to this bug, I noticed an issue where if the height of the window is reduced too much, a console log error occurs. This error is shown in the console -
Since this issue deals with the window resizing, I feel it would be better to tackle this here. The solution for this is -
diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index 1d9922ead4..a1c98784a1 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -82,7 +82,8 @@ const AvatarCropModal = (props) => {
const initializeImageContainer = useCallback((event) => {
setIsImageContainerInitialized(true);
const {height, width} = event.nativeEvent.layout;
- setImageContainerSize(Math.floor(Math.min(height - styles.imageCropRotateButton.height, width)));
+ const relativeHeight = height - styles.imageCropRotateButton.height;
+ setImageContainerSize(Math.floor(Math.min(relativeHeight > 0 ? relativeHeight : 0, width)));
}, [props.isSmallScreenWidth]);
// An onLayout callback, that initializes the slider container size, for proper render of a slider
In addition to this bug, I noticed an issue where if the height of the window is reduced too much, a console log error occurs. Since this issue deals with the window resizing, I feel it would be better to tackle this here. The solution for this is -
diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js index 1d9922ead4..a1c98784a1 100644 --- a/src/components/AvatarCropModal/AvatarCropModal.js +++ b/src/components/AvatarCropModal/AvatarCropModal.js @@ -82,7 +82,8 @@ const AvatarCropModal = (props) => { const initializeImageContainer = useCallback((event) => { setIsImageContainerInitialized(true); const {height, width} = event.nativeEvent.layout; - setImageContainerSize(Math.floor(Math.min(height - styles.imageCropRotateButton.height, width))); + const relativeHeight = height - styles.imageCropRotateButton.height; + setImageContainerSize(Math.floor(Math.min(relativeHeight > 0 ? relativeHeight : 0, width))); }, [props.isSmallScreenWidth]); // An onLayout callback, that initializes the slider container size, for proper render of a slider
I think that right minimum value of imageContainerSize isn't 0 but CONST.AVATAR_CROP_MODAL.INITIAL_SIZE
s77rt
commented
1 year ago @alexxxwork I like your solution.
Math.max instead of Math.abs (compare height - styles.imageCropRotateButton.height with 0)updateImageOffset(translateX.value,translateY.value); does not preserve the translation values. We should either:
(0, 0) for a consist behaviour.Something like that
diff --git a/src/components/AvatarCropModal/AvatarCropModal.js b/src/components/AvatarCropModal/AvatarCropModal.js
index 1d9922ead4..cc3dd5fbea 100644
--- a/src/components/AvatarCropModal/AvatarCropModal.js
+++ b/src/components/AvatarCropModal/AvatarCropModal.js
@@ -82,7 +82,8 @@ const AvatarCropModal = (props) => {
const initializeImageContainer = useCallback((event) => {
setIsImageContainerInitialized(true);
const {height, width} = event.nativeEvent.layout;
- setImageContainerSize(Math.floor(Math.min(height - styles.imageCropRotateButton.height, width)));
+ setImageContainerSize(Math.floor(Math.min(Math.max(height - styles.imageCropRotateButton.height, 0), width)));
+ updateImageOffset(0, 0);
}, [props.isSmallScreenWidth]);
// An onLayout callback, that initializes the slider container size, for proper render of a sliderResting the translation values is not that terrible it fixes the reported bug, but it would be nicer to be able to calculate new translation values that gives us the same displayed portion of the image.
alexxxwork
commented
1 year ago @alexxxwork I like your solution.
Thanks for the suggestion @s77rt! Updated my proposal!
parasharrajat
commented
1 year ago I will review the proposals in some time.
parasharrajat
commented
1 year ago Oops! apologies for the delay here. I missed it somehow. I will check all the proposals after dinner.
abekkala
commented
1 year ago @parasharrajat have you had a chance to review the proposals here?
alexxxwork
commented
1 year ago I think I should repost my proposal with actual template. So I've updated my first comment https://github.com/Expensify/App/issues/14712#issuecomment-1412696977
parasharrajat
commented
1 year ago Thanks, @alexxxwork. I was going to ask the same.
@abekkala I will share that review today in a couple of hours. There was some freezing issue with my Mac which I have just fixed. So I should be good to continue. Most probably, we will have this finalized today.
parasharrajat
commented
1 year ago Ok, here is the initial feedback.
so far I understood @alexxxwork proposal better than others so I only have feedback on that. But I will share feedback on other proposals once you have explained your code changes.
@alexxxwork I haven't checked/tested but I think there might be issues with your proposed changes. We might get inconsistent translate values in the onLayout event.
onLayout is fired in react-cycle but all the animations are running natively from react-native-reanimated. There might be chances where data is not synced between both events.
Should I first post a root cause analysis or it's not the case?
Please explain both root cause and its solution in your proposal.
I think that right minimum value of imageContainerSize isn't 0 but CONST.AVATAR_CROP_MODAL.INITIAL_SIZE
I will search for that.
alexxxwork
commented
1 year ago There might be chances where data is not synced between both events.
@parasharrajat What do you mean by data inconsistency? I've updated explanation details of my proposal https://github.com/Expensify/App/issues/14712#issuecomment-1412696977 The logic here is so that it works before any avatar image is loaded (loader animation is displayed) so there's no inconsistency.
parasharrajat
commented
1 year ago Ok, checking it again in some time. Still waiting for others to update their proposals.
parasharrajat
commented
1 year ago Ok. Back to it and checking.
MelvinBot
commented
1 year ago @abekkala @MariaHCD @parasharrajat 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!
abekkala
commented
1 year ago @parasharrajat do you have further concerns/questions for the proposal that was provided by @alexxxwork?
Ollyws
commented
1 year ago Resizing the window when editing an avatar causes the image to move out of bounds causing a server error when submitted.
This is caused because: First we are setting the imageOffset every time we pan or use the slider.
Then when we resize the window the onLayout of GestureHandlerRootView is called and subsequently the imageContainerSize is changed, however the imageOffset is never updated to reflect the new imageContainerSize causing the alignment issues we see.
We already have a useEffect here which is used to align the slider when the window size changes.
We can take the slider value and use it to call sliderOnPress (or at least some of the code inside it) within the useEffect, which will update the imageOffset according to the new values.
Another further modification we need to make is to update this line to accomodate NaN values as sometimes, when the view gets really small both values are zero resulting in NaN (which leaves the window blank).
It can be fixed by adding || 0 like so:
return (((newSliderValue / containerSize) || 0) * (MAX_SCALE - MIN_SCALE)) + MIN_SCALE;We already have a
useEffecthere which is used to align the slider when the window size changes.
As I can see the only difference of this proposal is to place logic in existing useEffect to AvatarCropModal instead of initializeImageContainer callback to GestureHandlerRootView - so it's placing the logic in upper component
parasharrajat
commented
1 year ago @abekkala Can you please revert the price to 2K? There are proposals that need to be fully reviewed. I will be able to finalize this one by tomorrow. It seems they have what it needs to solve this issue. Just need to check for side-effects.
Prince-Mendiratta
commented
1 year ago In this issue, when the browser window is resized, the current translateX and translateY values aren't maintained and clamped, leading to the image crop out of bounds and thus, the error.
When the modal is initialised, the sliderContainerSize value is set according to the layout of the window. When the image is zoomed in, the scale value increases, also increasing the image width and height, calculated using thegetDisplayedImageSize function and depend on the imageContainerSize. When we try to pan across the image in increased scale, the translateX and translateY value changes according to the current maxOffsetX and maxOffsetY respectively depending on the imageContainerSize. Now, the issue is that when the browser window size changes, the imageContainerSize and the image proportions change. However, the offset values do not change. Thus, this leads to the case where the old offset values becomes large enough to exceed the bounds of the image. This causes an empty image to be sent to the servers, leading to the error.
To solve this problem, we need to update the offset values when the browser resizes. To do this, we will use a useEffect, that trigger when the imageContainerSize changes. Now, we need to update the offset values, translateX and translateY. If we do this, we will notice that when the browser window is reduced, the offset values will reach the boundary of the clamp and be set like that as we reduce the boundaries. When we increase it again though, the offset value do not increase in proportion to the maxOffset values, leading to image not being zoomed out correctly when the browser size increases.
To fix this, we need to keep a track of the previous offset values. To do this, I will create two new shared values, prevMaxOffsetX and prevMaxOffsetY and set their initial value to 0.
Next, I will update their value when we modify the scale. So we need to update their values to the maxOffsetX and maxOffsetY respectively in updateImageOffset function.
With this, we are set to finally interpolate over the previous offset and current offset values to get the final offset values. This can be done by specifying the previous offset values in the input array and the current offset values in the output array. With this, we can ensure that the proportionate value of the offset is maintained.
If it is hard to understand logically only (I relate, maths is hard), I can share the code snippet as well for this. Will make things much easier to understand.
While testing this issue, I noticed that on certain browsers, you can reduce the browser height / width to pretty much bare minimum. This leads to a console error showing as follows:
This issue can be resolved by setting the minimum possible value for the sliderContainer to be CONST.AVATAR_CROP_MODAL.INITIAL_SIZE in initializeImageContainer.
setImageContainerSize(Math.max(Math.floor(Math.min(height - styles.imageCropRotateButton.height, width)), CONST.AVATAR_CROP_MODAL.INITIAL_SIZE));To fix this, we need to keep a track of the previous offset values. To do this, I will create two new shared values,
prevMaxOffsetXandprevMaxOffsetYand set their initial value to 0.
@Prince-Mendiratta could you explain why do you need these previous values? The only problem is that avatar image position doesn't respond to layout changes of the parent element.
abekkala
commented
1 year ago @Prince-Mendiratta can you address the question above by @alexxxwork
Prince-Mendiratta
commented
1 year ago Sure!
why do you need these previous values?
This is to maintain the ratio of translation value when compared to the maximum offset values (which depend on the container size), especially when the window is first reduced in size and then expanded. This will preserve the current position that the user has navigated to. This will resolve all the problems mentioned by @s77rt in this comment.
The only problem is that avatar image position doesn't respond to layout changes of the parent element.
Yeah, that would be the problem in a nutshell. What is required from the proposal is the ideal solution which would be something like no matter if you reduce / expand the window, the current position that you've cropped to remains the same. I tried out what you proposed but it did not maintain the current position of the cropped area.
To explain the idea better, here's a step by step breakdown of the problem:
Furthermore, in addition to my current proposal, I will also add the maximum bounds allowed post cropping to make sure that in the future, even if there are any issues (which are unlikely), there wouldn't be any empty image sent to the server and thus, no errors other than possible minor visual glitch.
@abekkala @alexxxwork
parasharrajat
commented
1 year ago Apologies for missing this issue. I will get it today. @abekkala There is a request for https://github.com/Expensify/App/issues/14712#issuecomment-1432041468.
ETA: 6 hours.
alexxxwork
commented
1 year ago This is to maintain the ratio of translation value when compared to the maximum offset values
@Prince-Mendiratta this makes sense, but it's more like UX enchancement than a bugfix :) Overall, I think it's a great improvement.
MelvinBot
commented
1 year ago Upwork job price has been updated to $2000
MelvinBot
commented
1 year ago @abekkala @MariaHCD @parasharrajat 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!
MelvinBot
commented
1 year ago @abekkala, @MariaHCD, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
parasharrajat
commented
1 year ago This is a low-priority issue. I will get back to it asap. I expect that I will be able to check it tomorrow.
Need to test a few scenarios before I can be assured of proposals.
MariaHCD
commented
1 year ago I'll catch up here but let me know what your thoughts are and how I can help, @parasharrajat!
MelvinBot
commented
1 year ago Upwork job price has been updated to $2000
parasharrajat
commented
1 year ago I am not working today but I will either get to it tomorrow or reassign it. My bucket list is almost over.
MelvinBot
commented
1 year ago @abekkala @MariaHCD @parasharrajat this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks!
MelvinBot
commented
1 year ago Current assignee @parasharrajat is eligible for the Internal assigner, not assigning anyone new.
alexxxwork
commented
1 year ago @parasharrajat Is this issue going to be internal now? 😨
mountiny
commented
1 year ago @alexxxwork no, it should not be internal until the proposals are considered
parasharrajat
commented
1 year ago Checking it in 2 hours.
parasharrajat
commented
1 year ago Looking it...Working on and off for two days and till Monday.
MelvinBot
commented
1 year ago @abekkala, @MariaHCD, @parasharrajat Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
abekkala
commented
1 year ago @parasharrajat how are you doing here? is there anything that @MariaHCD can help you with?
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:
Expected Result:
Image is trimmed properly and saved properly
Actual Result:
Image goes out of bounds, sends the out of bounds picture to the server and we get a 502 server error (with potentially sensitive info)
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.63-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:
https://user-images.githubusercontent.com/43996225/215887038-cf9f69e2-ae29-4312-9458-77031ba9cd29.mov
https://user-images.githubusercontent.com/43996225/215887257-f4fccb69-2353-4df0-a5a6-8892c1e84b9f.mp4
Expensify/Expensify Issue URL: Issue reported by: @oesayan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674723830727639
View all open jobs on GitHub
Upwork Automation - Do Not Edit