[RNMobile] Update Image component to avoid flicker when updating the URI #57869
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +1.42 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
derekblank
added
[Type] Enhancement
… behavior between platforms
|
Flaky tests detected in 5c4d2d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7707646158
|
dcalhoun
reviewed
Jan 19, 2024
There was a problem hiding this comment.
This turned out to be a complex subject. 😅 From my initial testing, the functionality works as described. Well done! 👏🏻
I did notice that replacing media for an Image or Gallery block does not show the newly selected local image immediately. Instead, it displays the previous image until the upload completes, it then swaps to the new image instantly. I'm not sure if this an existing issue or not.
I also noticed an image can flash when the upload finishes in a Gallery block, but only on occasion. It occurs sometimes when I upload ~4 images to a new Gallery block. It could be unrelated to the Gallery block and I just happen to recreate it more frequently with Gallery block given it generally houses multiple images.
Resolves layout display issue when block is selected
@dcalhoun Good catch, I also noted that the newly selected local image does not immediately replace the local file until it's uploaded to the server. While it still prevents the flickering, I think it's worth seeing if we can improve the replace behavior as well. |
dcalhoun
reviewed
Jan 22, 2024
The mobile image component introduce new asynchronous effects. We must await the result of those effect, in the form of asserting UI changes from the subsequent state updates, to satisfy the React test utilities expectation that unexpected re-renders do not occur after the test completes. Additionally, there were instances of awaiting `act` invocations that were not asynchronous. The erroneous usage of `await` for these synchronous `act` calls resulted in cryptic errors. In reality, the logic run within these `act` calls are synchronous and should merely be wrapped in `act` to signal that they result in a re-render.
Using width: 1 and height: 1 ensures that onLoad will fire
…://github.com/wordpress/gutenberg into rnmobile/prevent-image-flicker-when-uploading
dcalhoun
reviewed
Jan 25, 2024
There was a problem hiding this comment.
I did not note any new regressions in my testing. During my testing I noted:
- [Known Regression] Replacing an image does not display the new image until the upload is completed, which is a regression that we may choose to ship. I left an inline comment regarding a potential solution.
- On iOS only, Media + Text and Cover blocks do not display the local image during the upload, but this appears to be true in the production app as well.
- Choosing a photo from a "free photo library" does not result in a smooth transition.
Co-authored-by: David Calhoun <github@davidcalhoun.me>
…://github.com/wordpress/gutenberg into rnmobile/prevent-image-flicker-when-uploading
dcalhoun
approved these changes
Jan 30, 2024
There was a problem hiding this comment.
The proposed changes look good to me.
I have not retested with the latest commits after #57869 (review), as there is not an updated prototype build. However, if you'd like for me to re-test something, please let me know.
I will also note that we should re-test for any Android regressions you noted in #57869 (comment).
Thank you for improving this image upload experience!
On iOS, add 1 to height to account for the 1px non-visible image that is used to determine when the network image has loaded We also must verify that it is not NaN, as it can be NaN when the image is loading.
I ran through the regression tests for both platforms, and did not note any with the updated logic. Your logic change also improved the replacement behavior for Android -- thanks for the contribution! I did note that when an image is selected on iOS, there is a 1px offset between the border and the image due to the non-visible image. I noted this as something to fix in a previous comment, and updated the Before | AfterNote the 1px offset on the bottom of the selected image in the "Before" example:
|
|
@dcalhoun I have started the main apps build CI jobs to reference the latest changes, and tested both platforms successfully locally. Although you've already approved, I just wanted to provide a final chance for any review from the last change to update the selected image styles, and general testing using the prototype builds (which should have succeeded by the time you read this). Tomorrow I intend to test the main apps CI builds and if successful, merge the Gutenberg / Gutenberg Mobile PRs before cutting the 1.112.0 release. Thanks for your feedback and contributions to this fix. 🙇 |
dcalhoun
approved these changes
Jan 31, 2024
This was referenced Feb 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes an issue where image uploads disappear or "blink" when transitioning from local to network states.
Related PRs:
Why?
Fixes:
Currently when uploading an Image, the Image itself "blinks" when transitioning its URI reference from a local on-device image to the uploaded, network-available image. This "blink" is possibly due to the time spent downloading the network-available resource itself, where the visual asset appears unavailable while the network image is downloading.
How?
Adds more descriptive states when a
uriis changed to await the full loading of a network-based image, before swapping out the URI from the local filestore-based image.Testing Instructions
Screenshots or screencast