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

[RNMobile] add selectionStart and selectionEnd to transientEdits #22492

Merged
merged 3 commits into from
May 25, 2020

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented May 20, 2020

Description

Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2285

In this PR i added a selectionStart and selectionEnd to transient Edits. The motivation is described here: wordpress-mobile/gutenberg-mobile#2192 (comment)

I also added a debounce function to create an undo level when the input (rich-text) stops for over a second to be consistent with the web:
https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/component/index.js#L501

It changes how the undo mechanism works on mobile.

Known Issues:

These issues are not introduced by this PR

  • Sometimes the redo level is removed in rich-text - After testing, I can say that this PR doesn't introduce that issue but it is easier to reproduce. (GIFS in the testing section) I found that it happens only when the paragraph block has the break line at the very beginning. It's really edge case because as far as I know, it is possible by adding it to the code editor.

  • We create unnecessary undo levels on the editor init - [RNMobile] add selectionStart and selectionEnd to transientEdits #22492 (comment)

How has this been tested?

Undo/redo block actions

  • From the example app, remove a few blocks.
  • Press the Undo button to see them reappear.
  • Press Redo to remove the blocks again.

Undo/redo text
NOTE: This test case doesn't work as before. The whole text is removed since we do not create the undo level when we change the same attribute in the same block. Before changes from this PR the undo level was created after each sign in the rich-text. After these changes, the undo works the same as on the web.

Before After
undobefore undoafter
Web
undowebpar
  • Write some text on a text based block .
  • Press Undo until all new text has disappeared.
  • Press Redo to see the text appear again.

Edit: I observed a weird behavior. Sometimes when i remove text by pressing on the undo button the redo level is not created - see the gif below (the redo button is disabled after pressing undo)

Issue
paraissue

Undo/redo text format
In that case, i have the same issue as in the previous one - the redo level is removed in rich-text sometimes

Issue
boldissue
  • On a rich-text based component, add some format (bold, links, etc…).
  • Undo all changes and Redo them to arrive to the same state.

I've tested that and it seems like these issues i mentioned above also appear w/o changes from this PR

Types of changes

Add selectionStart and selectionEnd to transientEdits to make undo mechanism work the same as on the web

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented May 20, 2020

Size Change: +205 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-library/index.js 119 kB +94 B (0%)
build/edit-widgets/index.js 7.87 kB +141 B (1%)
build/edit-widgets/style-rtl.css 4.58 kB -15 B (0%)
build/edit-widgets/style.css 4.57 kB -15 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.24 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 13.2 kB 0 B
build/edit-site/style-rtl.css 5.45 kB 0 B
build/edit-site/style.css 5.45 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@dratwas
Copy link
Contributor Author

dratwas commented May 21, 2020

I thought that these changes (selectionStart and selectionEnd) are a cause of these issues but no, they are just easier to reproduce but appears also in the current state of the app. During the investigation, I found that the undo level (or few of them) is created on the init of the editor which I think is a bug. An interesting thing is that if the post/page doesn't contain some InnerBlocks then only one undo level is created and pressing on the undo button removes all blocks. In case you have i.e Cover block, then two undo levels are created and first remove the inner blocks from the Cover block and then remove all blocks.

No inner blocks With inner blocks
undoinit initbig

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

I tested this on Pixel 3a and this PR works as described. I confirmed too that the issues mentioned are present in develop, and not introduced in this PR. I also observed another issue (which is also present on develop) where the caret did not "land" in the correct place after undo and redo actions:

Nice work on this PR @dratwas , and thanks for documenting your findings with great detail! 😄

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

Tested on iOS iPhone 11 works as described for me as well 👍

@dratwas dratwas merged commit 1e76458 into master May 25, 2020
@dratwas dratwas deleted the rnmobile/fix/undo-levels branch May 25, 2020 09:59
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 25, 2020
@mchowning
Copy link
Contributor

👋 It looks like this change has introduced a regression with respect to saving a post when someone is typing quickly: wordpress-mobile/gutenberg-mobile#2349.

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

Successfully merging this pull request may close these issues.

None yet

4 participants