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

RNMobile: Remove willTrimSpacesCheck for Android #22006

Closed
wants to merge 1 commit into from
Closed

RNMobile: Remove willTrimSpacesCheck for Android #22006

Show file tree
Hide file tree
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
36 changes: 1 addition & 35 deletions packages/rich-text/src/component/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ export class RichText extends Component {
this
);
this.valueToFormat = this.valueToFormat.bind( this );
this.willTrimSpaces = this.willTrimSpaces.bind( this );
this.getHtmlToRender = this.getHtmlToRender.bind( this );
this.state = {
activeFormats: [],
Expand Down Expand Up @@ -631,29 +630,6 @@ export class RichText extends Component {
}
}

willTrimSpaces( html ) {
const { tagName } = this.props;

// aztec won't trim spaces in a case of <pre> block, so we are excluding it
if ( tagName === 'pre' ) {
return false;
}

// regex for detecting spaces around block element html tags
const blockHtmlElements =
'(div|br|blockquote|ul|ol|li|p|pre|h1|h2|h3|h4|h5|h6|iframe|hr)';
const leadingOrTrailingSpaces = new RegExp(
`(\\s+)<\/?${ blockHtmlElements }>|<\/?${ blockHtmlElements }>(\\s+)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @mchowning, looks like the whitespace preserving code on the Aztec side is preserving simple spaces only, while the regex here is checking for all whitespace chars. I'm under the impression that the Aztec side behavior won't actually preserve all whitespace chars, so, can you check as well?

I tried having a tab char (ASCII 0x09) leading and trialing the test but couldn't break the editor (the whitespace was preserved) although I'm not sure I did actually manage to insert the TAB, it might actually had become a regular space char.

Copy link
Contributor Author

@mchowning mchowning May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for taking a look @hypest!

looks like the whitespace preserving code on the Aztec side is preserving simple spaces only, while the regex here is checking for all whitespace chars.

Are you aware of any specific reason the regex is checking for all whitespace instead of just spaces. I wasn't able to find a reason that was needed (both the comment on the regex and the original change with adding this regex seemed to only involve issues with space characters specifically.

I'm under the impression that the Aztec side behavior won't actually preserve all whitespace chars, so, can you check as well?

I tried having a tab char (ASCII 0x09) leading and trialing the test but couldn't break the editor (the whitespace was preserved) although I'm not sure I did actually manage to insert the TAB, it might actually had become a regular space char.

I noticed the same thing, a lot of non-standard characters I tried (tab, line feed, carriage return, non-breaking space) were converted into spaces (well, eventually: line feed would briefly be a newline and non-breaking space would briefly be &nbsp;, but those would then get turned into spaces). In everything I tried, I couldn't find anything that broke on Gutenberg.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you aware of any specific reason the regex is checking for all whitespace instead of just spaces.

If I recall correctly, the whitespace in general gets removed by Aztec because HTML itself is ignoring them, and Aztec tries to follow that logic. In this case, I think I tried to generalize and having in mind that the AztecParser.tidy() method and perhaps other components too will try to remove some whitespace.

'g'
);
const matches = html.match( leadingOrTrailingSpaces );
if ( matches && matches.length > 0 ) {
return true;
}

return false;
}

getHtmlToRender( record, tagName ) {
// Save back to HTML from React tree
let value = this.valueToFormat( record );
Expand Down Expand Up @@ -722,17 +698,7 @@ export class RichText extends Component {

// On AztecAndroid, setting the caret to an out-of-bounds position will crash the editor so, let's check for some cases.
if ( ! this.isIOS ) {
// Aztec performs some html text cleanup while parsing it so, its internal representation gets out-of-sync with the
// representation of the format-lib on the RN side. We need to avoid trying to set the caret position because it may
// be outside the text bounds and crash Aztec, at least on Android.
if ( this.willTrimSpaces( html ) ) {
// the html will get trimmed by the cleaning up functions in Aztec and caret position will get out-of-sync.
// So, skip forcing it, let Aztec just do its best and just log the fact.
console.warn(
'RichText value will be trimmed for spaces! Avoiding setting the caret position manually.'
);
selection = null;
} else if (
if (
this.props.selectionStart > record.text.length ||
this.props.selectionEnd > record.text.length
) {
Expand Down
34 changes: 0 additions & 34 deletions packages/rich-text/src/component/test/index.native.js
View file
Open in desktop

This file was deleted.