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.

Sign up for GitHub

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

Rich text: fix props on DOM element warnings #31883

Merged
merged 1 commit into from
May 18, 2021
Merged

Rich text: fix props on DOM element warnings #31883

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 29 additions & 0 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,34 @@ import { useFormatTypes } from './use-format-types';
import FormatEdit from './format-edit';
import { getMultilineTag, getAllowedFormats } from './utils';

/**
* Removes props used for the native version of RichText so that they are not
* passed to the DOM element and log warnings.
*
* @param {Object} props Props to filter.
*
* @return {Object} Filtered props.
*/
function removeNativeProps( props ) {
return omit( props, [
'__unstableMobileNoFocusOnMount',
'deleteEnter',
'placeholderTextColor',
'textAlign',
'selectionColor',
Copy link
Member Author

Choose a reason for hiding this comment

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

Should a block be able to set selection color?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do still want this though as it enables a bit of control over color accessibility. The problem is that the selection highlight box and cursor can blend into the block's background color when we only use the platform's Native color choice. For the case of buttons, we have it set to match the text color as an attempt to provide some contrast.

Here's the original issue that was opened for this to get some more context wordpress-mobile/gutenberg-mobile#1773

'tagsToEliminate',
'rootTagsToEliminate',
Copy link
Member Author

Choose a reason for hiding this comment

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

What does this do? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

These props are used to clean up any undesired tags produced by Aztec (the native renderer we use for rich text), here is the related code:

gutenberg/packages/rich-text/src/component/index.native.js

Lines 247 to 266 in c290539

/*
* Cleans up any root tags produced by aztec.
* TODO: This should be removed on a later version when aztec doesn't return the top tag of the text being edited
*/
removeRootTagsProduceByAztec( html ) {
let result = this.removeRootTag( this.props.tagName, html );
// Temporary workaround for https://github.com/WordPress/gutenberg/pull/13763
if ( this.props.rootTagsToEliminate ) {
this.props.rootTagsToEliminate.forEach( ( element ) => {
result = this.removeRootTag( element, result );
} );
}
if ( this.props.tagsToEliminate ) {
this.props.tagsToEliminate.forEach( ( element ) => {
result = this.removeTag( element, result );
} );
}
return result;
}

'disableEditingMenu',
Copy link
Member Author

Choose a reason for hiding this comment

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

What does this do? What is being disabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used to control triggering the suggestions by keycodes, besides for iOS, this value also determines whether the user can edit the attributes of the text in the text field.

'fontSize',
'fontFamily',
'fontWeight',
'fontStyle',
'minWidth',
'maxWidth',
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a style equivalent for Native to avoid all these style props?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in theory we could extract them from the style object that is passed to the native view 🤔 .

'setRef',
Copy link
Member Author

Choose a reason for hiding this comment

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

Could we use ref here instead?

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 use ref we would get the reference of the RichText component, right? If that's the case, it won't work because here what we need is the reference of the underneath native view of Aztec (RCTAztecView).

Copy link
Member Author

Choose a reason for hiding this comment

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

Would forwardRef not work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, yeah, we could forward the ref upstream instead of using setRef 👍 .

] );
}

function RichTextWrapper(
{
children,
Expand Down Expand Up @@ -75,6 +103,7 @@ function RichTextWrapper(
const instanceId = useInstanceId( RichTextWrapper );

identifier = identifier || instanceId;
props = removeNativeProps( props );

const anchorRef = useRef();
const { clientId } = useBlockEditContext();
Expand Down