From db7278a01ca170f523f8fab99bcf29823d3366eb Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Thu, 4 Jun 2020 07:41:47 -0700 Subject: [PATCH 01/11] External Media: Render Media after authentication (#16044) See https://github.com/Automattic/jetpack/pull/15717#pullrequestreview-418201700 --- .../sources/google-photos/google-photos-auth.js | 8 +++----- .../external-media/sources/google-photos/index.js | 7 +------ .../shared/external-media/sources/with-media.js | 13 ++++++++----- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/extensions/shared/external-media/sources/google-photos/google-photos-auth.js b/extensions/shared/external-media/sources/google-photos/google-photos-auth.js index a5830852ee083..a1cf2c7d2e22a 100644 --- a/extensions/shared/external-media/sources/google-photos/google-photos-auth.js +++ b/extensions/shared/external-media/sources/google-photos/google-photos-auth.js @@ -20,8 +20,7 @@ import AuthInstructions from './auth-instructions'; import AuthProgress from './auth-progress'; function GooglePhotosAuth( props ) { - const { getMedia } = props; - + const { setAuthenticated } = props; const [ isAuthing, setIsAuthing ] = useState( false ); const onAuthorize = useCallback( () => { @@ -39,15 +38,14 @@ function GooglePhotosAuth( props ) { // Open authorize URL in a window and let it play out requestExternalAccess( service.connect_URL, () => { setIsAuthing( false ); - const url = getApiUrl( 'list', SOURCE_GOOGLE_PHOTOS ); - getMedia( url, true ); + setAuthenticated( true ); } ); } ) .catch( () => { // Not much we can tell the user at this point so let them try and auth again setIsAuthing( false ); } ); - }, [ getMedia ] ); + }, [ setAuthenticated ] ); return (
diff --git a/extensions/shared/external-media/sources/google-photos/index.js b/extensions/shared/external-media/sources/google-photos/index.js index b387cfd7147a1..8581abe9041d0 100644 --- a/extensions/shared/external-media/sources/google-photos/index.js +++ b/extensions/shared/external-media/sources/google-photos/index.js @@ -1,8 +1,3 @@ -/** - * WordPress dependencies - */ -import { __ } from '@wordpress/i18n'; - /** * Internal dependencies */ @@ -11,7 +6,7 @@ import GooglePhotosAuth from './google-photos-auth'; import GooglePhotosMedia from './google-photos-media'; function GooglePhotos( props ) { - if ( props.requiresAuth ) { + if ( ! props.isAuthenticated ) { return ; } diff --git a/extensions/shared/external-media/sources/with-media.js b/extensions/shared/external-media/sources/with-media.js index cf1bebc87499e..0040676a896b4 100644 --- a/extensions/shared/external-media/sources/with-media.js +++ b/extensions/shared/external-media/sources/with-media.js @@ -48,11 +48,13 @@ export default function withMedia() { nextHandle: false, isLoading: false, isCopying: null, - requiresAuth: false, + isAuthenticated: true, path: { ID: PATH_RECENT }, }; } + setAuthenticated = isAuthenticated => this.setState( { isAuthenticated } ); + mergeMedia( initial, media ) { return uniqBy( initial.concat( media ), 'ID' ); } @@ -88,7 +90,7 @@ export default function withMedia() { handleApiError = error => { if ( error.code === 'authorization_required' ) { - this.setState( { requiresAuth: true, isLoading: false, isCopying: false } ); + this.setState( { isAuthenticated: false, isLoading: false, isCopying: false } ); return; } @@ -121,7 +123,7 @@ export default function withMedia() { const path = this.getRequestUrl( url ); const method = 'GET'; - this.setState( { requiresAuth: false } ); + this.setAuthenticated( true ); apiFetch( { path, @@ -174,7 +176,7 @@ export default function withMedia() { } renderContent() { - const { media, isLoading, nextHandle, requiresAuth, path } = this.state; + const { media, isLoading, nextHandle, isAuthenticated, path } = this.state; const { noticeUI, allowedTypes, multiple = false } = this.props; return ( @@ -189,7 +191,8 @@ export default function withMedia() { media={ media } pageHandle={ nextHandle } allowedTypes={ allowedTypes } - requiresAuth={ requiresAuth } + isAuthenticated={ isAuthenticated } + setAuthenticated={ this.setAuthenticated } multiple={ multiple } path={ path } onChangePath={ this.onChangePath } From d65219c2bf6b5f1b7a52b0ea2792566acaefc996 Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Thu, 4 Jun 2020 07:51:58 -0700 Subject: [PATCH 02/11] [not verified] Handle 401 responses (#16041) See https://github.com/Automattic/jetpack/pull/15717#pullrequestreview-418201700 --- .../class-wpcom-rest-api-v2-endpoint-external-media.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php b/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php index 09f095d3ca524..5741bccd27b94 100644 --- a/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php +++ b/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php @@ -231,6 +231,14 @@ function( $key ) { $response = json_decode( wp_remote_retrieve_body( $response ) ); break; + case 401: + $response = new WP_Error( + 'authorization_required', + __( 'You are not connected to that service.', 'jetpack' ), + array( 'status' => 403 ) + ); + break; + case 403: $error = json_decode( wp_remote_retrieve_body( $response ) ); $response = new WP_Error( $error->code, $error->message, $error->data ); From 0d26c39e05284647a5ebcbc3762db3ec10b945a0 Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Fri, 5 Jun 2020 09:15:05 -0700 Subject: [PATCH 03/11] External Media: Better reflect accepted media types (#16071) --- .../external-media/media-button/media-menu.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/extensions/shared/external-media/media-button/media-menu.js b/extensions/shared/external-media/media-button/media-menu.js index f40d8111e3723..55cb1b0aa8b0c 100644 --- a/extensions/shared/external-media/media-button/media-menu.js +++ b/extensions/shared/external-media/media-button/media-menu.js @@ -13,6 +13,10 @@ function MediaButtonMenu( props ) { const { mediaProps, open, setSelectedSource, isFeatured, isReplace } = props; const originalComponent = mediaProps.render; + if ( isFeatured && mediaProps.value === undefined ) { + return originalComponent( { open } ); + } + if ( isReplace ) { return ( 1 + ? __( 'Select Media', 'jetpack' ) + : __( 'Select Image', 'jetpack' ); return ( <> @@ -55,11 +60,11 @@ function MediaButtonMenu( props ) { aria-expanded={ isOpen } onClick={ onToggle } > - { __( 'Select Image', 'jetpack' ) } + { label } ) } renderContent={ ( { onToggle } ) => ( - + openLibrary( onToggle ) }> { __( 'Media Library', 'jetpack' ) } From a1c5142d9a30e4fafd81c62e272613553c29ffab Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Tue, 9 Jun 2020 03:14:58 -0700 Subject: [PATCH 04/11] External Media: Account for multiple images (#16077) See https://github.com/Automattic/jetpack/pull/16071#issuecomment-639367271 --- .../external-media/media-button/media-menu.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/extensions/shared/external-media/media-button/media-menu.js b/extensions/shared/external-media/media-button/media-menu.js index 55cb1b0aa8b0c..e4c57389ee1c3 100644 --- a/extensions/shared/external-media/media-button/media-menu.js +++ b/extensions/shared/external-media/media-button/media-menu.js @@ -40,10 +40,15 @@ function MediaButtonMenu( props ) { open(); }; - const label = - mediaProps.allowedTypes.length > 1 - ? __( 'Select Media', 'jetpack' ) - : __( 'Select Image', 'jetpack' ); + let label = __( 'Select Image', 'jetpack' ); + + if ( mediaProps.multiple ) { + label = __( 'Select Images', 'jetpack' ); + } + + if ( mediaProps.allowedTypes.length > 1 ) { + label = __( 'Select Media', 'jetpack' ); + } return ( <> From 81f731c957205b46cd720ee1d8dc43cb47e15f21 Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Tue, 9 Jun 2020 16:10:34 +0200 Subject: [PATCH 05/11] External Media: Fix Modal focus loss on arrow keypress (#16055) Co-authored-by: Marek Hrabe --- .../external-media/sources/with-media.js | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/extensions/shared/external-media/sources/with-media.js b/extensions/shared/external-media/sources/with-media.js index 0040676a896b4..635809fe3325f 100644 --- a/extensions/shared/external-media/sources/with-media.js +++ b/extensions/shared/external-media/sources/with-media.js @@ -11,6 +11,7 @@ import apiFetch from '@wordpress/api-fetch'; import { createHigherOrderComponent } from '@wordpress/compose'; import { Component } from '@wordpress/element'; import { withNotices, Modal } from '@wordpress/components'; +import { UP, DOWN, LEFT, RIGHT } from '@wordpress/keycodes'; import { __ } from '@wordpress/i18n'; /** @@ -53,6 +54,42 @@ export default function withMedia() { }; } + modalRef = el => { + if ( el ) { + // Find the modal wrapper. + this.modalElement = el.closest( '.jetpack-external-media-browser' ); + + // Attach the listener if found. + if ( this.modalElement ) { + this.modalElement.addEventListener( 'keydown', this.stopArrowKeysPropagation ); + } + } else if ( this.modalElement ) { + // Remove listeners when unmounting. + this.modalElement.removeEventListener( 'keydown', this.stopArrowKeysPropagation ); + this.modalElement = null; + } + }; + + stopArrowKeysPropagation = event => { + /** + * When the External Media modal is open, pressing any arrow key causes + * it to close immediately. This is happening because the keydown event + * propagates outside the modal, triggering a re-render and a blur event + * eventually. We could avoid that by isolating the modal from the Image + * block render scope, but it is not possible in current implementation. + * + * This handler makes sure that the keydown event doesn't propagate further, + * which fixes the issue described above while still keeping arrow keys + * functional inside the modal. + * + * This can be removed once + * https://github.com/WordPress/gutenberg/issues/22940 is fixed. + */ + if ( [ UP, DOWN, LEFT, RIGHT ].includes( event.keyCode ) ) { + event.stopPropagation(); + } + }; + setAuthenticated = isAuthenticated => this.setState( { isAuthenticated } ); mergeMedia( initial, media ) { @@ -171,17 +208,13 @@ export default function withMedia() { this.setState( { path }, cb ); }; - stopPropagation( event ) { - event.stopPropagation(); - } - renderContent() { const { media, isLoading, nextHandle, isAuthenticated, path } = this.state; const { noticeUI, allowedTypes, multiple = false } = this.props; return ( // eslint-disable-next-line jsx-a11y/no-static-element-interactions -
+
{ noticeUI } - { isCopying ? : this.renderContent() } +
+ { isCopying ? : this.renderContent() } +
); } From 6a924423b9e921ce7e47f8a0fc0fed9cf2e7f4bb Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Wed, 10 Jun 2020 10:49:06 -0700 Subject: [PATCH 06/11] External Media: Add API unit tests (#15988) * [not verified] Add External media endpoint * [not verified] Add unit tests * [not verified] Add class coverage info * Simplify schema * Fix image copy test * Use local image * Remove duplicate file ending Props @frontdevde. --- ...om-rest-api-v2-endpoint-external-media.php | 52 ++- ...om-rest-api-v2-endpoint-external-media.php | 337 ++++++++++++++++++ 2 files changed, 362 insertions(+), 27 deletions(-) create mode 100644 tests/php/core-api/wpcom-endpoints/test_class-wpcom-rest-api-v2-endpoint-external-media.php diff --git a/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php b/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php index 5741bccd27b94..116317eeebd13 100644 --- a/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php +++ b/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-external-media.php @@ -21,34 +21,32 @@ class WPCOM_REST_API_V2_Endpoint_External_Media extends WP_REST_Controller { * @var array */ public $media_schema = array( - 'items' => array( - 'type' => 'object', - 'required' => true, - 'properties' => array( - 'caption' => array( - 'type' => 'string', - ), - 'guid' => array( - 'items' => array( - 'caption' => array( - 'type' => 'string', - ), - 'name' => array( - 'type' => 'string', - ), - 'title' => array( - 'type' => 'string', - ), - 'url' => array( - 'format' => 'uri', - 'type' => 'string', - ), + 'type' => 'object', + 'required' => true, + 'properties' => array( + 'caption' => array( + 'type' => 'string', + ), + 'guid' => array( + 'items' => array( + 'caption' => array( + 'type' => 'string', + ), + 'name' => array( + 'type' => 'string', + ), + 'title' => array( + 'type' => 'string', + ), + 'url' => array( + 'format' => 'uri', + 'type' => 'string', ), - 'type' => 'array', - ), - 'title' => array( - 'type' => 'string', ), + 'type' => 'array', + ), + 'title' => array( + 'type' => 'string', ), ), ); @@ -120,7 +118,7 @@ public function register_routes() { 'args' => array( 'media' => array( 'description' => __( 'Media data to copy.', 'jetpack' ), - 'items' => array_values( $this->media_schema ), + 'items' => $this->media_schema, 'required' => true, 'type' => 'array', 'sanitize_callback' => array( $this, 'sanitize_media' ), diff --git a/tests/php/core-api/wpcom-endpoints/test_class-wpcom-rest-api-v2-endpoint-external-media.php b/tests/php/core-api/wpcom-endpoints/test_class-wpcom-rest-api-v2-endpoint-external-media.php new file mode 100644 index 0000000000000..c076ddff76393 --- /dev/null +++ b/tests/php/core-api/wpcom-endpoints/test_class-wpcom-rest-api-v2-endpoint-external-media.php @@ -0,0 +1,337 @@ +user->create( array( 'role' => 'administrator' ) ); + static::$image_path = dirname( dirname( __DIR__ ) ) . '/files/jetpack.jpg'; + } + + /** + * Setup the environment for a test. + */ + public function setUp() { + parent::setUp(); + + wp_set_current_user( static::$user_id ); + + add_filter( 'pre_option_jetpack_private_options', array( $this, 'mock_jetpack_private_options' ) ); + } + + /** + * Reset the environment to its original state after the test. + */ + public function tearDown() { + remove_filter( 'pre_option_jetpack_private_options', array( $this, 'mock_jetpack_private_options' ) ); + + parent::tearDown(); + } + + /** + * Tests empty list response. + */ + public function test_list_pexels_empty() { + add_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_list_pexels' ), 10, 3 ); + + $request = wp_rest_request( Requests::GET, '/wpcom/v2/external-media/list/pexels' ); + $response = $this->server->dispatch( $request ); + $data = $response->get_data(); + + $this->assertObjectHasAttribute( 'found', $data ); + $this->assertObjectHasAttribute( 'media', $data ); + $this->assertObjectHasAttribute( 'meta', $data ); + $this->assertObjectHasAttribute( 'next_page', $data->meta ); + $this->assertEmpty( $data->media ); + + remove_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_list_pexels' ) ); + } + + /** + * Tests list response with unauthenticated Google Photos. + */ + public function test_list_google_photos_unauthenticated() { + add_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_list_google_photos_unauthenticated' ), 10, 3 ); + + $request = wp_rest_request( Requests::GET, '/wpcom/v2/external-media/list/google_photos' ); + $response = $this->server->dispatch( $request ); + $error = $response->get_data(); + + $this->assertObjectHasAttribute( 'code', $error ); + $this->assertObjectHasAttribute( 'message', $error ); + $this->assertObjectHasAttribute( 'data', $error ); + $this->assertEquals( 'authorization_required', $error->code ); + $this->assertEquals( 403, $error->data->status ); + + remove_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_list_google_photos_unauthenticated' ) ); + } + + /** + * Tests list response with unauthenticated Google Photos. + */ + public function test_copy_image() { + $tmp_name = $this->get_temp_name( static::$image_path ); + if ( file_exists( $tmp_name ) ) { + unlink( $tmp_name ); + } + + add_filter( 'pre_http_request', array( $this, 'mock_image_data' ), 10, 3 ); + + add_filter( + 'wp_handle_sideload_prefilter', + function( $file ) { + copy( static::$image_path, $file['tmp_name'] ); + + return $file; + } + ); + add_filter( + 'wp_check_filetype_and_ext', + function() { + return array( + 'ext' => 'jpg', + 'type' => 'image/jpeg', + 'proper_filename' => basename( static::$image_path ), + ); + } + ); + + $request = new WP_REST_Request( Requests::POST, '/wpcom/v2/external-media/copy/pexels' ); + $request->set_body_params( + array( + 'media' => array( + array( + 'guid' => wp_json_encode( + array( + 'url' => static::$image_path, + 'name' => $this->image_name, + ) + ), + ), + ), + ) + ); + $response = $this->server->dispatch( $request ); + $data = $response->get_data()[0]; + remove_filter( 'pre_http_request', array( $this, 'mock_image_data' ) ); + + $this->assertArrayHasKey( 'id', $data ); + $this->assertArrayHasKey( 'caption', $data ); + $this->assertArrayHasKey( 'alt', $data ); + $this->assertArrayHasKey( 'type', $data ); + $this->assertArrayHasKey( 'url', $data ); + $this->assertEquals( 'image', $data['type'] ); + $this->assertInternalType( 'int', $data['id'] ); + $this->assertEmpty( $data['caption'] ); + $this->assertEmpty( $data['alt'] ); + } + + /** + * Tests connection response for Google Photos. + */ + public function test_connection_google_photos() { + add_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_connection_google_photos' ), 10, 3 ); + $request = wp_rest_request( Requests::GET, '/wpcom/v2/external-media/connection/google_photos' ); + $response = $this->server->dispatch( $request ); + $data = $response->get_data(); + + $this->assertEquals( 'google_photos', $data->ID ); + $this->assertNotEmpty( $data->connect_URL ); // phpcs:ignore + + remove_filter( 'pre_http_request', array( $this, 'mock_wpcom_api_response_connection_google_photos' ) ); + } + + /** + * Mock the user token. + * + * @return array + */ + public function mock_jetpack_private_options() { + return array( + 'user_tokens' => array( + static::$user_id => 'pretend_this_is_valid.secret.' . static::$user_id, + ), + ); + } + + /** + * Validate the "list" Jetpack API request for Pexels and mock the response. + * + * @param bool $response Whether to preempt an HTTP request's return value. Default false. + * @param array $args HTTP request arguments. + * @param string $url The request URL. + * @return array + */ + public function mock_wpcom_api_response_list_pexels( $response, $args, $url ) { + $this->assertEquals( Requests::GET, $args['method'] ); + $this->assertStringStartsWith( 'https://public-api.wordpress.com/wpcom/v2/meta/external-media/pexels', $url ); + + return array( + 'headers' => array( + 'Allow' => 'GET', + ), + 'body' => '{"found":0,"media":[],"meta":{"next_page":false}}', + 'status' => 200, + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + } + + /** + * Validate the "list" Jetpack API request for Google Photos and mock the response. + * + * @param bool $response Whether to preempt an HTTP request's return value. Default false. + * @param array $args HTTP request arguments. + * @param string $url The request URL. + * @return array + */ + public function mock_wpcom_api_response_list_google_photos_unauthenticated( $response, $args, $url ) { + $this->assertEquals( Requests::GET, $args['method'] ); + $this->assertStringStartsWith( 'https://public-api.wordpress.com/wpcom/v2/meta/external-media/google_photos', $url ); + + return array( + 'headers' => array( + 'Allow' => 'GET', + ), + 'body' => '{"code":"authorization_required","message":"You are not connected to that service.","data":{"status":403}}', + 'status' => 403, + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + } + + /** + * Validate the "copy" Jetpack API request for Pexels and mock the response. + * + * @param bool $response Whether to preempt an HTTP request's return value. Default false. + * @param array $args HTTP request arguments. + * @param string $url The request URL. + * @return array + */ + public function mock_image_data( $response, $args, $url ) { + $this->assertEquals( static::$image_path, $url ); + + return array( + 'headers' => array( + 'Allow' => 'GET', + ), + 'status' => 200, + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + } + + /** + * Validate the "connection" Jetpack API request for Google Photos and mock the response. + * + * @param bool $response Whether to preempt an HTTP request's return value. Default false. + * @param array $args HTTP request arguments. + * @param string $url The request URL. + * @return array + */ + public function mock_wpcom_api_response_connection_google_photos( $response, $args, $url ) { + $this->assertEquals( WP_REST_Server::READABLE, $args['method'] ); + $this->assertEquals( 'https://public-api.wordpress.com/wpcom/v2/meta/external-media/connection/google_photos', $url ); + + return array( + 'headers' => array( + 'Allow' => 'GET', + ), + 'body' => '{"ID":"google_photos","label":"Google Photos","type":"other","description":"Access photos in your Google Account for use in posts and pages","genericon":{"class":"googleplus-alt","unicode":"\\f218"},"icon":"http:\/\/i.wordpress.com\/wp-content\/lib\/external-media-service\/icon\/google-photos-2x.png","connect_URL":"https:\/\/public-api.wordpress.com\/connect\/?action=request&kr_nonce=0&nonce=0&for=connect&service=google_photos&kr_blog_nonce=0&magic=keyring&blog=0","multiple_external_user_ID_support":false,"external_users_only":false,"jetpack_support":true}', + 'status' => 200, + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + } + + /** + * Re-creates a temporary file name so we can clean up after ourselves. + * + * @param string $filename File name. + * @param string $dir Temp directory. Dafault empty. + * + * @return string|string[]|null + */ + public function get_temp_name( $filename, $dir = '' ) { + if ( empty( $dir ) ) { + $dir = get_temp_dir(); + } + + if ( empty( $filename ) || in_array( $filename, array( '.', '/', '\\' ), true ) ) { + $filename = uniqid(); + } + + // Use the basename of the given file without the extension as the name for the temporary directory. + $temp_filename = basename( $filename ); + $temp_filename = preg_replace( '|\.[^.]*$|', '', $temp_filename ); + + // If the folder is falsey, use its parent directory name instead. + if ( ! $temp_filename ) { + return wp_tempnam( dirname( $filename ), $dir ); + } + + // Suffix some random data to avoid filename conflicts. + $temp_filename .= '-' . wp_generate_password( 6, false ); + $temp_filename .= '.tmp'; + + add_filter( 'wp_unique_filename', array( $this, 'get_file_name' ) ); + $temp_filename = $dir . wp_unique_filename( $dir, $temp_filename ); + remove_filter( 'wp_unique_filename', array( $this, 'get_file_name' ) ); + + return $temp_filename; + } + + /** + * Filter callback to provide a similar file name as in tested class. + * + * @see WPCOM_REST_API_V2_Endpoint_External_Media::tmp_name() + * + * @return string + */ + public function get_file_name() { + return $this->image_name; + } +} From 3fa86139cce66da13bbc0b0c8198bdf684b93e6c Mon Sep 17 00:00:00 2001 From: Konstantin Obenland Date: Wed, 10 Jun 2020 11:38:58 -0700 Subject: [PATCH 07/11] External Media: Improve Insert Flow (#16081) * [not verified] # This is a combination of 5 commits. External Media: Improve Insert Flow See https://github.com/Automattic/jetpack/issues/15867 Remove unused import Lock pointer events when copying This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending. Announce Inserting action along with alt text of selected images. Update modal headers to reflect status Props @jancavan. * External Media: Fix mobile modal size (#16098) * External Media: Improve Insert Flow See https://github.com/Automattic/jetpack/issues/15867 * Remove unused import * Lock pointer events when copying This prevents a UX issue where user can still interact with the modal content while the "Inserting..." request is pending. * Announce Inserting action along with alt text of selected images. * Fix mobile mobile width to take up the full available modal space * Change is-copying to modifier instead of element to follow BEM syntax Co-authored-by: Konstantin Obenland Co-authored-by: Bart * External Media: Loading and disabled states for the copying state (#16103) * disable pagination while loading * fade out all images when copying * disable event handlers and mark as disabled when copying * disable pexels search when copying * disabled google filters when copying * add text to copying indicator * disable google photos view selector when copying * [not verified] remove debugging condition * Pass functions not booleans to event emitters Co-authored-by: Konstantin Obenland * Removed :not() copying css state since they are the same modal now * Reset focus to the modal after clicking Insert. Otherwise focus is reset to the body and focus needs to stay trapped within the modal. * Cosmetically rearranging code order * External Media: Screen Reader accessible states for Insert flow (#16113) * Add visually hidden description for the external media modal to give improved screen reader instructions and communicate the copying state * [not verified] Remove title attribute from grid images * [not verified] Move Inserting Image... copy to before the image so it is announced first. * Separate-out description and element id Helps with keeping describedby id in sync Co-authored-by: Konstantin Obenland Co-authored-by: Jerry Jones Co-authored-by: Bart Co-authored-by: Marek Hrabe --- extensions/shared/external-media/editor.scss | 94 ++++++--------- .../external-media/media-browser/index.js | 42 +++++-- .../media-browser/media-item.js | 23 ++-- .../sources/google-photos/filter-view.js | 6 +- .../google-photos/google-photos-media.js | 4 +- .../shared/external-media/sources/pexels.js | 8 +- .../external-media/sources/with-media.js | 110 +++++++++--------- 7 files changed, 154 insertions(+), 133 deletions(-) diff --git a/extensions/shared/external-media/editor.scss b/extensions/shared/external-media/editor.scss index 2e9c9cc9c9b4a..95e82cd61a2ea 100644 --- a/extensions/shared/external-media/editor.scss +++ b/extensions/shared/external-media/editor.scss @@ -17,44 +17,20 @@ $grid-size: 8px; } } +.jetpack-external-media-browser--visually-hidden { + position: absolute !important; + height: 1px; + width: 1px; + overflow: hidden; + clip: rect( 1px, 1px, 1px, 1px ); + white-space: nowrap; /* added line */ +} + /** * Media item container */ -.jetpack-external-media-browser__is-copying { - max-height: 20%; - max-width: 20%; - min-width: 480px; - min-height: 360px + 56px; - - .jetpack-external-media-browser__single { - height: 300px; - } - - .jetpack-external-media-browser__media { - height: 100%; - } - - .is-transient { - border: 0; - background: 0; - padding: 0; - height: 100%; - - &.jetpack-external-media-browser__media__item__selected { - box-shadow: none; - border-radius: 0; - } - - img { - width: 100%; - height: 100%; - object-fit: contain; - } - } -} - -.jetpack-external-media-browser:not( .jetpack-external-media-browser__is-copying ) { +.jetpack-external-media-browser { .is-error { margin-bottom: 1em; margin-left: 0; @@ -71,29 +47,16 @@ $grid-size: 8px; } } -@media ( min-width: 600px ) { - .jetpack-external-media-browser .components-modal__content { - width: 90vw; - height: 90vh; - } +.jetpack-external-media-browser--is-copying { + pointer-events: none; } -.jetpack-external-media-browser__single { - position: relative; - display: flex; - justify-content: center; - align-items: center; - - .is-transient img { - opacity: 0.3; - } +.components-modal__content { + width: 100%; - .components-spinner { - position: absolute; - top: 50%; - right: 50%; - margin-top: -9px; - margin-right: -9px; + @media ( min-width: 600px ) { + width: 90vw; + height: 90vh; } } @@ -132,16 +95,29 @@ $grid-size: 8px; &.is-transient img { opacity: 0.3; } + } + + .jetpack-external-media-browser__media__copying_indicator { + display: flex; + position: absolute; + top: 0; + left: 0; + width: 100%; + height: 100%; + flex-direction: column; + justify-content: center; + align-items: center; + text-align: center; .components-spinner { - position: absolute; - top: 50%; - right: 50%; - margin-top: -9px; - margin-right: -9px; + margin-bottom: $grid-size; } } + .jetpack-external-media-browser__media__copying_indicator__label { + font-size: 12px; + } + .jetpack-external-media-browser__media__folder { float: left; display: flex; diff --git a/extensions/shared/external-media/media-browser/index.js b/extensions/shared/external-media/media-browser/index.js index 76898ca93058c..1eb207290b0d0 100644 --- a/extensions/shared/external-media/media-browser/index.js +++ b/extensions/shared/external-media/media-browser/index.js @@ -25,7 +25,17 @@ const EmptyResults = memo( () => ( ) ); function MediaBrowser( props ) { - const { media, isLoading, pageHandle, className, multiple, setPath, nextPage, onCopy } = props; + const { + media, + isCopying, + isLoading, + pageHandle, + className, + multiple, + setPath, + nextPage, + onCopy, + } = props; const [ selected, setSelected ] = useState( [] ); const onSelectImage = useCallback( @@ -70,6 +80,25 @@ function MediaBrowser( props ) { nextPage(); }; + const SelectButton = () => { + const disabled = selected.length === 0 || isCopying; + const label = isCopying ? __( 'Inserting…', 'jetpack' ) : __( 'Select', 'jetpack' ); + + return ( +
+ +
+ ); + }; + return (
    @@ -80,6 +109,7 @@ function MediaBrowser( props ) { onClick={ onSelectImage } focusOnMount={ !! prevMediaCount.current && index === prevMediaCount.current } isSelected={ selected.find( toFind => toFind.ID === item.ID ) } + isCopying={ isCopying } /> ) ) } @@ -91,7 +121,7 @@ function MediaBrowser( props ) { isLarge isSecondary className="jetpack-external-media-browser__loadmore" - disabled={ isLoading } + disabled={ isLoading || isCopying } onClick={ onLoadMoreClick } > { __( 'Load More', 'jetpack' ) } @@ -99,13 +129,7 @@ function MediaBrowser( props ) { ) }
- { hasMediaItems && ( -
- -
- ) } + { hasMediaItems && }
); } diff --git a/extensions/shared/external-media/media-browser/media-item.js b/extensions/shared/external-media/media-browser/media-item.js index 503dcf3969326..20cfaa0d230b6 100644 --- a/extensions/shared/external-media/media-browser/media-item.js +++ b/extensions/shared/external-media/media-browser/media-item.js @@ -9,6 +9,7 @@ import classnames from 'classnames'; import { useRef, useEffect, useCallback } from '@wordpress/element'; import { Spinner } from '@wordpress/components'; import { ENTER, SPACE } from '@wordpress/keycodes'; +import { __ } from '@wordpress/i18n'; function MediaItem( props ) { const onClick = useCallback( () => { @@ -34,7 +35,7 @@ function MediaItem( props ) { 'jetpack-external-media-browser__media__item': true, 'jetpack-external-media-browser__media__item__selected': isSelected, 'jetpack-external-media-browser__media__folder': type === 'folder', - 'is-transient': isSelected && isCopying, + 'is-transient': isCopying, } ); const itemEl = useRef( null ); @@ -52,13 +53,23 @@ function MediaItem( props ) {
  • - { + { isSelected && isCopying && ( +
    + +
    + { __( 'Inserting Image…', 'jetpack' ) } +
    +
    + ) } + + { { type === 'folder' && (
    @@ -66,8 +77,6 @@ function MediaItem( props ) {
    { children }
    ) } - - { isSelected && isCopying && }
    • ); } diff --git a/extensions/shared/external-media/sources/google-photos/filter-view.js b/extensions/shared/external-media/sources/google-photos/filter-view.js index 0bf78a369afec..2f6384c8f61c1 100644 --- a/extensions/shared/external-media/sources/google-photos/filter-view.js +++ b/extensions/shared/external-media/sources/google-photos/filter-view.js @@ -44,7 +44,7 @@ function addFilter( existing, newFilter ) { function GoogleFilterView( props ) { const [ currentFilter, setCurrentFilter ] = useState( getFirstFilter( [] ) ); - const { isLoading, filters, canChangeMedia } = props; + const { isLoading, isCopying, filters, canChangeMedia } = props; const remainingFilters = removeMediaType( getFilterOptions( filters ), canChangeMedia ); const setFilter = () => { const newFilters = addFilter( filters, currentFilter ); @@ -62,12 +62,12 @@ function GoogleFilterView( props ) { - diff --git a/extensions/shared/external-media/sources/google-photos/google-photos-media.js b/extensions/shared/external-media/sources/google-photos/google-photos-media.js index b033675f99acc..7673283d7d679 100644 --- a/extensions/shared/external-media/sources/google-photos/google-photos-media.js +++ b/extensions/shared/external-media/sources/google-photos/google-photos-media.js @@ -21,6 +21,7 @@ const isImageOnly = allowed => allowed && allowed.length === 1 && allowed[ 0 ] = function GooglePhotosMedia( props ) { const { media, + isCopying, isLoading, pageHandle, multiple, @@ -85,7 +86,7 @@ function GooglePhotosMedia( props ) { className="jetpack-external-media-header__select" label={ __( 'View', 'jetpack' ) } value={ path.ID !== PATH_RECENT ? PATH_ROOT : PATH_RECENT } - disabled={ isLoading } + disabled={ isLoading || isCopying } options={ PATH_OPTIONS } onChange={ setPath } /> @@ -118,6 +119,7 @@ function GooglePhotosMedia( props ) { className="jetpack-external-media-browser__google" key={ listUrl } media={ media } + isCopying={ isCopying } isLoading={ isLoading } nextPage={ getNextPage } onCopy={ onCopy } diff --git a/extensions/shared/external-media/sources/pexels.js b/extensions/shared/external-media/sources/pexels.js index 0e0c1c8c8d192..ee8f1d5d57ebe 100644 --- a/extensions/shared/external-media/sources/pexels.js +++ b/extensions/shared/external-media/sources/pexels.js @@ -15,7 +15,7 @@ import MediaBrowser from '../media-browser'; import { getApiUrl } from './api'; function PexelsMedia( props ) { - const { media, isLoading, pageHandle, multiple, copyMedia, getMedia } = props; + const { media, isCopying, isLoading, pageHandle, multiple, copyMedia, getMedia } = props; const [ searchQuery, setSearchQuery ] = useState( sample( PEXELS_EXAMPLE_QUERIES ) ); const [ lastSearchQuery, setLastSearchQuery ] = useState( '' ); @@ -88,12 +88,15 @@ function PexelsMedia( props ) { type="search" value={ searchQuery } onChange={ setSearchQuery } + disabled={ !! isCopying } /> @@ -103,6 +106,7 @@ function PexelsMedia( props ) { key={ lastSearchQuery } className="jetpack-external-media-browser__pexels" media={ media } + isCopying={ isCopying } isLoading={ isLoading } nextPage={ getNextPage } onCopy={ onCopy } diff --git a/extensions/shared/external-media/sources/with-media.js b/extensions/shared/external-media/sources/with-media.js index 635809fe3325f..632ac78c19555 100644 --- a/extensions/shared/external-media/sources/with-media.js +++ b/extensions/shared/external-media/sources/with-media.js @@ -11,35 +11,18 @@ import apiFetch from '@wordpress/api-fetch'; import { createHigherOrderComponent } from '@wordpress/compose'; import { Component } from '@wordpress/element'; import { withNotices, Modal } from '@wordpress/components'; +import { __, sprintf } from '@wordpress/i18n'; +import { speak } from '@wordpress/a11y'; import { UP, DOWN, LEFT, RIGHT } from '@wordpress/keycodes'; -import { __ } from '@wordpress/i18n'; /** * Internal dependencies */ import { PATH_RECENT } from '../constants'; -import MediaItem from '../media-browser/media-item'; - -const CopyingMedia = ( { items } ) => { - const classname = - items.length === 1 - ? 'jetpack-external-media-browser__single' - : 'jetpack-external-media-browser'; - - return ( -
    -
    - { items.map( item => ( - - ) ) } -
    -
    - ); -}; export default function withMedia() { return createHigherOrderComponent( OriginalComponent => { - // Grandfathered class as it was ported from an older codebase. + // Legacy class as it was ported from an older codebase. class WithMediaComponent extends Component { constructor( props ) { super( props ); @@ -181,6 +164,24 @@ export default function withMedia() { this.setState( { isCopying: items } ); this.props.noticeOperations.removeAllNotices(); + // If we have a modal element set, focus it. + // Otherwise focus is reset to the body instead of staying within the Modal. + if ( this.modalElement ) { + this.modalElement.focus(); + } + + // Announce the action with appended string of all the images' alt text. + speak( + sprintf( + __( 'Inserting: %s', 'jetpack' ), + items + .map( item => item.title ) + .filter( item => item ) + .join( ', ' ) + ), + 'polite' + ); + apiFetch( { path: apiUrl, method: 'POST', @@ -208,49 +209,54 @@ export default function withMedia() { this.setState( { path }, cb ); }; - renderContent() { - const { media, isLoading, nextHandle, isAuthenticated, path } = this.state; - const { noticeUI, allowedTypes, multiple = false } = this.props; - - return ( - // eslint-disable-next-line jsx-a11y/no-static-element-interactions -
    - { noticeUI } - - -
    - ); - } - render() { - const { isCopying } = this.state; - const { onClose } = this.props; - + const { isAuthenticated, isCopying, isLoading, media, nextHandle, path } = this.state; + const { allowedTypes, multiple = false, noticeUI, onClose } = this.props; + + const title = isCopying + ? __( 'Inserting media', 'jetpack' ) + : __( 'Select media', 'jetpack' ); + const description = isCopying + ? __( + 'When the media is finished copying and inserting, you will be returned to the editor.', + 'jetpack' + ) + : __( 'Select the media you would like to insert into the editor.', 'jetpack' ); + + const describedby = 'jetpack-external-media-browser__description'; const classes = classnames( { 'jetpack-external-media-browser': true, - 'jetpack-external-media-browser__is-copying': isCopying, + 'jetpack-external-media-browser--is-copying': isCopying, } ); return (
    - { isCopying ? : this.renderContent() } + { noticeUI } + +

    + { description } +

    + +
    ); From 2dc941f147e124417db607cd8954f13e1061dc0a Mon Sep 17 00:00:00 2001 From: Bart Kalisz Date: Thu, 11 Jun 2020 12:47:04 +0200 Subject: [PATCH 08/11] External Media: Restore focus on modal close (#16102) --- extensions/shared/external-media/editor.scss | 13 ++++++++++ .../external-media/media-button/media-menu.js | 25 ++++--------------- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/extensions/shared/external-media/editor.scss b/extensions/shared/external-media/editor.scss index 95e82cd61a2ea..45274ead5c7f6 100644 --- a/extensions/shared/external-media/editor.scss +++ b/extensions/shared/external-media/editor.scss @@ -26,6 +26,19 @@ $grid-size: 8px; white-space: nowrap; /* added line */ } +/** + * Media button menu + */ + +/** + * Hide the menu when the modal is open. Hacky, but necessary to allow the focus + * to return to selected option when modal is closed. This is how it's currenly + * also implemented in Gutenberg's MediaReplaceFlow. + */ +.modal-open .jetpack-external-media-button-menu__options { + display: none; +} + /** * Media item container */ diff --git a/extensions/shared/external-media/media-button/media-menu.js b/extensions/shared/external-media/media-button/media-menu.js index e4c57389ee1c3..367c75fe3007f 100644 --- a/extensions/shared/external-media/media-button/media-menu.js +++ b/extensions/shared/external-media/media-button/media-menu.js @@ -27,19 +27,6 @@ function MediaButtonMenu( props ) { ); } - const dropdownOpen = onToggle => { - onToggle(); - open(); - }; - const changeSource = ( source, onToggle ) => { - setSelectedSource( source ); - onToggle(); - }; - const openLibrary = onToggle => { - onToggle(); - open(); - }; - let label = __( 'Select Image', 'jetpack' ); if ( mediaProps.multiple ) { @@ -56,11 +43,12 @@ function MediaButtonMenu( props ) { (