Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resyncs RichText mobile components with web counterparts. #17897

Merged
merged 35 commits into from
Oct 29, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
ac7c363
Resyncs RichText mobile components with web counterparts.
SergioEstevao Oct 11, 2019
679f006
Remove outdated test.
SergioEstevao Oct 11, 2019
f52937c
Remove unused references.
SergioEstevao Oct 11, 2019
fe59886
Merge branch 'master' into rnmobile/rich_text_sync
SergioEstevao Oct 11, 2019
289bdb8
Add platform component
SergioEstevao Oct 22, 2019
f269ea5
Add components depending of platform.
SergioEstevao Oct 22, 2019
1fda754
Abstract paste of files for RN and web
SergioEstevao Oct 22, 2019
2491bee
Compose extra attributes/props on select/dispatch only if mobile.
SergioEstevao Oct 22, 2019
14a37cf
Remove RN index file for RichText Wrapper.
SergioEstevao Oct 22, 2019
b2ec21f
Remove API index native file that is no longer needed.
SergioEstevao Oct 24, 2019
ca1d308
Clean up lint errors in file-paste-handler.
SergioEstevao Oct 24, 2019
cd86c9c
Fix lint errors.
SergioEstevao Oct 24, 2019
f3f71d4
Implement stub remove browser shortcuts for RN
SergioEstevao Oct 24, 2019
51dec2a
Implement autocomplete stub for RN.
SergioEstevao Oct 24, 2019
d8e7807
Refactor toolbar presentation to a method.
SergioEstevao Oct 24, 2019
c5167db
Merge remote-tracking branch 'origin/master' into rnmobile/rich_text_…
SergioEstevao Oct 24, 2019
53194ff
Remove no longer needed platform file.
SergioEstevao Oct 24, 2019
d03b619
Consolidate the file paste handler in a single implementation.
SergioEstevao Oct 25, 2019
8fd2d6c
Change the text for platform to make it explicit it's native only.
SergioEstevao Oct 25, 2019
118d228
Remove duplicate files
ellatrix Oct 25, 2019
540ccd7
Include type in file comparison
ellatrix Oct 25, 2019
fdaaff0
Forgot to rename for native file
ellatrix Oct 25, 2019
fa4c573
Fix filePasteHandler for native
ellatrix Oct 25, 2019
07747d6
Move logging back
ellatrix Oct 25, 2019
9692574
Restore comment on logging
ellatrix Oct 25, 2019
345e655
Add check for files existence.
SergioEstevao Oct 25, 2019
cc9dab1
Refactor format-toolbar code to use split web/native files
SergioEstevao Oct 25, 2019
1b72134
Remove prop duplication.
SergioEstevao Oct 25, 2019
28ef938
Fix getAnchorRect call
SergioEstevao Oct 25, 2019
ce481aa
Remove unnecessary const
SergioEstevao Oct 26, 2019
014c931
Merge branch 'master' into rnmobile/rich_text_sync
SergioEstevao Oct 26, 2019
2cc0087
Sync fix for list removal of first empty line
SergioEstevao Oct 26, 2019
cf5ee3a
Fix RN build after merge with master
SergioEstevao Oct 26, 2019
b31067c
Sync with web counterpart.
SergioEstevao Oct 26, 2019
b5b433c
Only change selection after new formats are set.
SergioEstevao Oct 29, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* WordPress dependencies
*/
import { createBlobURL } from '@wordpress/blob';

export function filePasteHandler(images, html) {
if (!html && images.length > 0) {
let filesHTML = '';
images.forEach((image) => {
const file = image.getAsFile ? image.getAsFile() : image;
window.console.log('Received item:\n\n', file);
Copy link
Contributor

@koke koke Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought ESLint would complain about console.log statements, but I see it was already there. Maybe that's why it's using window.console? If that's the case and we want to keep it, using // eslint-disable-next-line no-console would be more explicit.

filesHTML += `<img src="${createBlobURL(file)}">`
});
return filesHTML;
} else {
return undefined
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* WordPress dependencies
*/
import { createBlobURL } from '@wordpress/blob';

SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved
export function filePasteHandler(images, html) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the differences with the web on this one, and what I'm seeing:

  • The native version adds a 'wp-image-{uploadId}' class. This seems related to the media upload in the apps, but I'm not sure if it's actually used, since pasting an image on RichText would end up transforming to an Image block, and that would request a media import anyway.
  • The web version receives a File object, while native receives paths to the files. I imagine we'll see more of this distinction elsewhere and I wonder if we should override @wordpress/blob instead so that our "blob" URLs are actually the passed file URLs (i.e. createBlobURL = url => url).
  • The web only processes files when there was no html pasted, but I think we could do the same.

Am I missing extra context, or could these two merge into a single implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native version adds a 'wp-image-{uploadId}' class. This seems related to the media upload in the apps, but I'm not sure if it's actually used, since pasting an image on RichText would end up transforming to an Image block, and that would request a media import anyway.

I think the reason the upload happens is because we set this wp-image-uploadId or else the image block will just refer to a local file. No?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with the whole process, but what I see on the image block is that it will requestMediaImport for any image with a file: URL. https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/edit.native.js#L107

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The web only processes files when there was no html pasted, but I think we could do the same.

Do you know the rational behind this? Why can't we have both?

If we want to do this on the native side too, we will need to check for empty string because our paste code sends an empty HTML string when no HTML is available: https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift#L245

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it was a fix for one of those “fun” bugs #4854

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to do this on the native side too, we will need to check for empty string

I think an empty string would evaluate as false, so the current check should work

if (images && images.length > 0) {
// Allows us to ask for this information when we get a report.
window.console.log( 'Received items:\n\n', images );
const uploadId = Number.MAX_SAFE_INTEGER;
let filesHTML = '';
images.forEach((file) => {
filesHTML += `<img src="${file}" class="wp-image-${uploadId}">`;
});
return filesHTML;
} else {
return undefined;
}

}
89 changes: 57 additions & 32 deletions packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import { omit } from 'lodash';
/**
* WordPress dependencies
*/
import { RawHTML, Component, createRef } from '@wordpress/element';
import { RawHTML, Component, createRef, Platform } from '@wordpress/element';
import { withDispatch, withSelect } from '@wordpress/data';
import { pasteHandler, children as childrenSource, getBlockTransforms, findTransform } from '@wordpress/blocks';
import { pasteHandler, children as childrenSource, getBlockTransforms, findTransform, isUnmodifiedDefaultBlock } from '@wordpress/blocks';
import { withInstanceId, compose } from '@wordpress/compose';
import {
__experimentalRichText as RichText,
Expand All @@ -26,7 +26,6 @@ import {
slice,
} from '@wordpress/rich-text';
import { withFilters, Popover } from '@wordpress/components';
import { createBlobURL } from '@wordpress/blob';
import deprecated from '@wordpress/deprecated';
import { isURL } from '@wordpress/url';

Expand All @@ -38,6 +37,7 @@ import BlockFormatControls from '../block-format-controls';
import FormatToolbar from './format-toolbar';
import { withBlockEditContext } from '../block-edit/context';
import { RemoveBrowserShortcuts } from './remove-browser-shortcuts';
import { filePasteHandler } from './file-paste-handler';

const wrapperClasses = 'editor-rich-text block-editor-rich-text';
const classes = 'editor-rich-text__editable block-editor-rich-text__editable';
Expand Down Expand Up @@ -124,7 +124,7 @@ class RichTextWrapper extends Component {
}
}

onPaste( { value, onChange, html, plainText, image } ) {
onPaste( { value, onChange, html, plainText, images } ) {
const {
onReplace,
onSplit,
Expand All @@ -134,16 +134,13 @@ class RichTextWrapper extends Component {
__unstableEmbedURLOnPaste,
} = this.props;

if ( image && ! html ) {
const file = image.getAsFile ? image.getAsFile() : image;
let imagesHTML = filePasteHandler(images, html)
if ( imagesHTML ) {
const content = pasteHandler( {
HTML: `<img src="${ createBlobURL( file ) }">`,
HTML: imagesHTML,
mode: 'BLOCKS',
tagName,
} );

// Allows us to ask for this information when we get a report.
window.console.log( 'Received item:\n\n', file );
} );

if ( onReplace && isEmpty( value ) ) {
onReplace( content );
Expand Down Expand Up @@ -429,7 +426,7 @@ class RichTextWrapper extends Component {
<FormatToolbar />
</BlockFormatControls>
) }
{ isSelected && inlineToolbar && hasFormats && (
{ Platform.OS === 'web' && isSelected && inlineToolbar && hasFormats && (
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved
<Popover
noArrow
position="top center"
Expand All @@ -440,25 +437,27 @@ class RichTextWrapper extends Component {
<FormatToolbar />
</Popover>
) }
{ isSelected && <RemoveBrowserShortcuts /> }
<Autocomplete
onReplace={ onReplace }
completers={ autocompleters }
record={ value }
onChange={ onChange }
isSelected={ isSelected }
>
{ ( { listBoxId, activeId, onKeyDown } ) =>
<Editable
aria-autocomplete={ listBoxId ? 'list' : undefined }
aria-owns={ listBoxId }
aria-activedescendant={ activeId }
start={ start }
reversed={ reversed }
onKeyDown={ onKeyDown }
/>
}
</Autocomplete>
{ Platform.OS === 'web' && isSelected && <RemoveBrowserShortcuts /> }
{ Platform.OS === 'web' && (
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved
<Autocomplete
onReplace={onReplace}
completers={autocompleters}
record={value}
onChange={onChange}
isSelected={isSelected}
>
{({ listBoxId, activeId, onKeyDown }) =>
<Editable
aria-autocomplete={listBoxId ? 'list' : undefined}
aria-owns={listBoxId}
aria-activedescendant={activeId}
start={start}
reversed={reversed}
onKeyDown={onKeyDown}
/>
}
</Autocomplete>
) }
</>
}
</RichText>
Expand All @@ -482,19 +481,31 @@ class RichTextWrapper extends Component {

const RichTextContainer = compose( [
withInstanceId,
withBlockEditContext( ( { clientId } ) => ( { clientId } ) ),
withBlockEditContext(({ clientId, onCaretVerticalPositionChange, isSelected }, ownProps) => {
if ( Platform.OS === 'web' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea to keep the current .native system: create the higher order component in a separate fire where it returns the same component for web, and import it here. Native could have two withBlockEditContext higher order components.

return { clientId }
} else {
return {
clientId,
blockIsSelected: ownProps.isSelected !== undefined ? ownProps.isSelected : isSelected,
onCaretVerticalPositionChange,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming we need these two, but it would be good to leave a comment here explaining the differences and why those are needed, which could make it easier to consolidate in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure why we needed those props, maybe @hypest or @Tug remember the reasons?

Copy link
Contributor

@hypest hypest Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see #13323 that introduced the onCaretVerticalPositionChange to fix a scrolling issue on mobile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And #15878 introduced blockIsSelected to fix a focus issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely look into it in a followup PR, there might be "cleaner" ways to address those issues

};
}
} ),
withSelect( ( select, {
clientId,
instanceId,
identifier = instanceId,
isSelected,
blockIsSelected,
} ) => {
const {
isCaretWithinFormattedText,
getSelectionStart,
getSelectionEnd,
getSettings,
didAutomaticChange,
__unstableGetBlockWithoutInnerBlocks,
} = select( 'core/block-editor' );

const selectionStart = getSelectionStart();
Expand All @@ -509,13 +520,27 @@ const RichTextContainer = compose( [
isSelected = selectionStart.clientId === clientId;
}

let extraProps = {}
if ( Platform.OS !== 'web' ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two props don't seem to conflict with the web stuff, so maybe we can skip the check and always inject them. Maybe we could have something like a __native prefix to indicate better that they are for native-specific behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the check for platform not make it obvious enough? I could change the code to Platform.OS == native` now that we have our Platform component in.

// If the block of this RichText is unmodified then it's a candidate for replacing when adding a new block.
// In order to fix https://github.com/wordpress-mobile/gutenberg-mobile/issues/1126, let's blur on unmount in that case.
// This apparently assumes functionality the BlockHlder actually
const block = clientId && __unstableGetBlockWithoutInnerBlocks(clientId);
const shouldBlurOnUnmount = block && isSelected && isUnmodifiedDefaultBlock(block);
extraProps = {
blockIsSelected,
SergioEstevao marked this conversation as resolved.
Show resolved Hide resolved
shouldBlurOnUnmount,
}
}

return {
canUserUseUnfilteredHTML: __experimentalCanUserUseUnfilteredHTML,
isCaretWithinFormattedText: isCaretWithinFormattedText(),
selectionStart: isSelected ? selectionStart.offset : undefined,
selectionEnd: isSelected ? selectionEnd.offset : undefined,
isSelected,
didAutomaticChange: didAutomaticChange(),
...extraProps,
};
} ),
withDispatch( ( dispatch, {
Expand Down
Loading