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

Issue/1966 Disable whitespace collapse in Aztec #2127

Merged
merged 17 commits into from
Apr 24, 2020

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Apr 8, 2020

Fixes #1966

Related PRs:

This PR fixes a bug where white space inserted at the start of text blocks ( Paragraph, Heading, Lists) were removed when applying a formatting option ( Bold, Italic, Strike).

The underlying issue was a sync issue between the content representation in the JS side and the Aztec content.

Aztec by default removes extra spaces (like most browsers do), but GB internal format doesn't do that, so when spaces were sent to Aztec they were removed and then when we apply formats Aztec will send the content back with all the extra space removed.

This PR links a new version of Aztec that allows disabling the removal/collapse of extra spaces.

To test:

  • Start the demo app
  • Add a text block, paragraph for example
  • Type some spaces, then write some text
  • Select the text and apply one format, for example, bold
  • Check that the white space aren't removed.
  • Repeat the test but without adding any text, just spaces, and then apply bold, to see if spaces are kept
  • Try to merge two paragraphs that have extra space at the start of the paragraph and on the end.
  • Check on the other blocks like Heading and Lists.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@SergioEstevao
Copy link
Contributor Author

@marecar3 and @hypest we will need someone from Aztec android to implement a similar option for disabling of the collapse of white spaces on Android.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 8, 2020

You can trigger optional full suite of UI tests for these changes by visiting CircleCI here.

1 similar comment
@peril-wordpress-mobile
Copy link

You can trigger optional full suite of UI tests for these changes by visiting CircleCI here.

@guarani
Copy link
Contributor

guarani commented Apr 8, 2020

Tests using Gutenberg example project on iPhone 7:

Copy Paste - Test Cases

  • TC001: Past formatted text copied from website

Multiline components - Test Cases

  • TC001: New line on multiline components

Rich Text Formatting - Test Cases

  • TC001: Bold, Italic, strikethrough buttons
  • TC002: Alignment buttons
  • TC003: Alignment split
  • TC004: Link button works without selection
  • TC005: Link button works with a selected word
  • TC006: Adding a link from a copied URL
  • TC007: Test format detection under the cursor

Splitting and merging - Test Cases

  • TC001: Merge after writing
  • TC002: Merge after selection
  • TC003: Merge after deleting text
  • TC004: Merging multiple blocks
  • TC004: Splitting/merge list block

@SergioEstevao, I noticed a tiny, but expected, difference in behavior here when splitting blocks: Previously, splitting blocks would strip any leading/trailing whitespace from the content that will populate the new block, but now, whitespace is preserved.

@SergioEstevao SergioEstevao changed the title Issue/1966 white space removal Issue/1966 Disable whitespace collapse in Aztec Apr 10, 2020
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Working great as expected! 🎉

@SergioEstevao, I noticed a tiny, but expected, difference in behavior here when splitting blocks: Previously, splitting blocks would strip any leading/trailing whitespace from the content that will populate the new block, but now, whitespace is preserved.

This is also the behaviour on Web, so I guess it's the expected one.

It would be great to add this corner case to the manual tests!

(Tested only on iOS)

@pinarol pinarol modified the milestones: 1.26, 1.27 Apr 13, 2020
@pinarol
Copy link
Contributor

pinarol commented Apr 13, 2020

I am changing the milestone of this PR as 1.27 because the 1.26 release branch will be cut quite soon. Let me know if you instead wanted to include this to 1.26.

@guarani
Copy link
Contributor

guarani commented Apr 13, 2020

It would be great to add this corner case to the manual tests!

@etoledom I'm not sure if this would be appropriate as a general observation in the splitting/merging test cases – let me know what you think. Otherwise, we could just leave it out since this new behavior feels quite normal.

@etoledom
Copy link
Contributor

etoledom commented Apr 14, 2020

It would be great to add this corner case to the manual tests!

@etoledom I'm not sure if this would be appropriate as a general observation in the splitting/merging test cases – let me know what you think. Otherwise, we could just leave it out since this new behavior feels quite normal.

Oh! I meant to add the main issue that this PR solves: "Adding format with many empty spaces at the beginning":

- Add a text block, paragraph for example
- Type some spaces, then write some text
- Select the text and apply one format, for example, bold
- Check that the white space aren't removed.

@guarani
Copy link
Contributor

guarani commented Apr 14, 2020

Oh! I meant to add the main issue that this PR solves: "Adding format with many empty spaces at the beginning":

@etoledom Gotcha! Sorry for the misunderstanding. I'll add that test case now. Added here.

@SergioEstevao, do you think I should run through more manual test cases on this? Otherwise I'd be happy to mark approved 💯.

@SergioEstevao
Copy link
Contributor Author

@guarani at the moment we are just waiting for @marecar3 to have some time to solve this in Android too.

@marecar3
Copy link
Contributor

@guarani at the moment we are just waiting for @marecar3 to have some time to solve this in Android too.

Hey @guarani @SergioEstevao
I have fixed the Android side, so @guarani you are able to test on both platforms. Thanks and sorry for the delay.

@guarani
Copy link
Contributor

guarani commented Apr 23, 2020

Thanks very much @marecar3! I'll give it a test.

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

This is looking great on both iOS and Android, thanks very much @marecar3 and @SergioEstevao!

I repeated the testing steps on both platforms, and smoke tested many of our blocks to try to catch any regressions and I found none.

I didn't run through all our test cases as this will be done when the release is cut.

@SergioEstevao
Copy link
Contributor Author

@marecar3 thanks for your work on this! If CI goes green I'm going to merge it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bolding text using the toolbar causes whitespace to be lost
5 participants