Skip to content

Commit

Permalink
[RNMobile] Update Image component to avoid flicker when updating the …
Browse files Browse the repository at this point in the history
…URI (#57869)

* Update native image component to avoid flicker when updating the URI

* Update non-visible Image block styles

* Update mobile Image block to use separate network and local image URL behavior between platforms

* Update Image block platform fetching logic

* Update Android logic for Image block platform fetching logic

* Add further updates to Image block platform logic

* Fix code formatting issue in Image block source prop

* Resolve RNImage.prefetch promise

* Update CHANGELOG

* Update non-visible image styles

Resolves layout display issue when block is selected

* Remove duplicate references to localURL

* Fix Replace Image behavior to update from localURL

* Refactor Image block URL handling logic

* test: Fix failures due to image component modifications (#58213)

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.

* Update nonVisibleImage styles

Using width: 1 and height: 1 ensures that onLoad will fire

* Update packages/components/src/mobile/image/index.native.js

Co-authored-by: David Calhoun <[email protected]>

* Update RNImage error handler event with code comment

* Update selected images styles on iOS to account for non-visible image

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.

---------

Co-authored-by: David Calhoun <[email protected]>
  • Loading branch information
derekblank and dcalhoun committed Jan 31, 2024
1 parent ceb3c1c commit 7b77931
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 17 deletions.
129 changes: 117 additions & 12 deletions packages/components/src/mobile/image/index.native.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { Image as RNImage, Text, View } from 'react-native';
import { Animated, Image as RNImage, Text, View } from 'react-native';
import FastImage from 'react-native-fast-image';

/**
Expand All @@ -11,7 +11,7 @@ import { __ } from '@wordpress/i18n';
import { Icon } from '@wordpress/components';
import { image, offline } from '@wordpress/icons';
import { usePreferredColorSchemeStyle } from '@wordpress/compose';
import { useEffect, useState, Platform } from '@wordpress/element';
import { useEffect, useState, useRef, Platform } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -54,6 +54,9 @@ const ImageComponent = ( {
} ) => {
const [ imageData, setImageData ] = useState( null );
const [ containerSize, setContainerSize ] = useState( null );
const [ localURL, setLocalURL ] = useState( null );
const [ networkURL, setNetworkURL ] = useState( null );
const [ networkImageLoaded, setNetworkImageLoaded ] = useState( false );

// Disabled for Android due to https://github.com/WordPress/gutenberg/issues/43149
const Image =
Expand All @@ -80,6 +83,33 @@ const ImageComponent = ( {
onImageDataLoad( metaData );
}
} );

if ( url.startsWith( 'file:///' ) ) {
setLocalURL( url );
setNetworkURL( null );
setNetworkImageLoaded( false );
} else if ( url.startsWith( 'https://' ) ) {
if ( Platform.isIOS ) {
setNetworkURL( url );
} else if ( Platform.isAndroid ) {
RNImage.prefetch( url ).then(
() => {
if ( ! isCurrent ) {
return;
}
setNetworkURL( url );
setNetworkImageLoaded( true );
},
() => {
// This callback is called when the image fails to load,
// but these events are handled by `isUploadFailed`
// and `isUploadPaused` events instead.
//
// Ignoring the error event will persist the local image URI.
}
);
}
}
}
return () => ( isCurrent = false );
// Disable reason: deferring this refactor to the native team.
Expand Down Expand Up @@ -188,9 +218,19 @@ const ImageComponent = ( {
focalPoint && styles.focalPointContainer,
];

const opacityValue = useRef( new Animated.Value( 1 ) ).current;

useEffect( () => {
Animated.timing( opacityValue, {
toValue: isUploadInProgress ? 0.3 : 1,
duration: 100,
useNativeDriver: true,
} ).start();
}, [ isUploadInProgress, opacityValue ] );

const imageStyles = [
{
opacity: isUploadInProgress ? 0.3 : 1,
opacity: opacityValue,
height: containerSize?.height,
},
! resizeMode && {
Expand All @@ -214,12 +254,29 @@ const ImageComponent = ( {
imageHeight && { height: imageHeight },
shapeStyle,
];

// 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.
// This is not necessary on Android as the non-visible image is not used.
let calculatedSelectedHeight;
if ( Platform.isIOS ) {
calculatedSelectedHeight =
containerSize && ! isNaN( containerSize.height )
? containerSize.height + 1
: 0;
} else {
calculatedSelectedHeight = containerSize?.height;
}

const imageSelectedStyles = [
usePreferredColorSchemeStyle(
styles.imageBorder,
styles.imageBorderDark
),
{ height: containerSize?.height },
{
height: calculatedSelectedHeight,
},
];

return (
Expand Down Expand Up @@ -259,14 +316,62 @@ const ImageComponent = ( {
</View>
) : (
<View style={ focalPoint && styles.focalPointContent }>
<Image
style={ imageStyles }
source={ { uri: url } }
{ ...( ! focalPoint && {
resizeMethod: 'scale',
} ) }
resizeMode={ imageResizeMode }
/>
{ Platform.isAndroid && (
<>
{ networkImageLoaded && networkURL && (
<Animated.Image
style={ imageStyles }
fadeDuration={ 0 }
source={ { uri: networkURL } }
{ ...( ! focalPoint && {
resizeMethod: 'scale',
} ) }
resizeMode={ imageResizeMode }
testID={ `network-image-${ url }` }
/>
) }
{ ! networkImageLoaded && ! networkURL && (
<Animated.Image
style={ imageStyles }
fadeDuration={ 0 }
source={ { uri: localURL } }
{ ...( ! focalPoint && {
resizeMethod: 'scale',
} ) }
resizeMode={ imageResizeMode }
/>
) }
</>
) }
{ Platform.isIOS && (
<>
<Animated.Image
style={ imageStyles }
source={ {
uri:
networkURL && networkImageLoaded
? networkURL
: localURL || url,
} }
{ ...( ! focalPoint && {
resizeMethod: 'scale',
} ) }
resizeMode={ imageResizeMode }
testID={ `network-image-${
networkURL && networkImageLoaded
? networkURL
: localURL || url
}` }
/>
<Image
source={ { uri: networkURL } }
style={ styles.nonVisibleImage }
onLoad={ () => {
setNetworkImageLoaded( true );
} }
/>
</>
) }
</View>
) }

Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/mobile/image/style.native.scss
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,9 @@
.wide {
width: 100%;
}

.nonVisibleImage {
height: 1;
width: 1;
opacity: 0;
}
13 changes: 8 additions & 5 deletions packages/edit-post/src/test/editor.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ describe( 'Editor', () => {
await initializeEditor();

// Act
await act( () => mediaAppendCallback( MEDIA[ 0 ] ) );
await act( () => mediaAppendCallback( MEDIA[ 2 ] ) );
act( () => mediaAppendCallback( MEDIA[ 0 ] ) );
act( () => mediaAppendCallback( MEDIA[ 2 ] ) );
await screen.findByTestId( `network-image-${ MEDIA[ 0 ].serverUrl }` );
await screen.findByTestId( `network-image-${ MEDIA[ 2 ].serverUrl }` );

// Assert
expect( getEditorHtml() ).toMatchSnapshot();
Expand All @@ -122,10 +124,11 @@ describe( 'Editor', () => {
await initializeEditor();

// Act
await act( () => mediaAppendCallback( MEDIA[ 0 ] ) );
act( () => mediaAppendCallback( MEDIA[ 0 ] ) );
// Unsupported type (PDF file)
await act( () => mediaAppendCallback( MEDIA[ 1 ] ) );
await act( () => mediaAppendCallback( MEDIA[ 3 ] ) );
act( () => mediaAppendCallback( MEDIA[ 1 ] ) );
act( () => mediaAppendCallback( MEDIA[ 3 ] ) );
await screen.findByTestId( `network-image-${ MEDIA[ 0 ].serverUrl }` );

// Assert
expect( getEditorHtml() ).toMatchSnapshot();
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ For each user feature we should also add a importance categorization label to i
- [**] Image block media uploads display a custom error message when there is no internet connection [#56937]
- [*] Fix missing custom color indicator for custom gradients [#57605]
- [**] Display a notice when a network connection unavailable [#56934]
- [**] Prevent images from temporarily disappearing when uploading media [#57869]

This comment has been minimized.

Copy link
@fluiddot

fluiddot Feb 1, 2024

Contributor

@derekblank seems this entry was added to the wrong section when merging.


## 1.110.1
- [**] Fix crash when RichText values are not defined [#58088]
Expand Down
8 changes: 8 additions & 0 deletions test/native/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,14 @@ jest.spyOn( Image, 'getSize' ).mockImplementation( ( url, success ) =>
success( 0, 0 )
);

jest.spyOn( Image, 'prefetch' ).mockImplementation(
( url, callback = () => {} ) => {
const mockRequestId = `mockRequestId-${ url }`;
callback( mockRequestId );
return Promise.resolve( true );
}
);

jest.mock( 'react-native/Libraries/Utilities/BackHandler', () => {
return jest.requireActual(
'react-native/Libraries/Utilities/__mocks__/BackHandler.js'
Expand Down

0 comments on commit 7b77931

Please sign in to comment.