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] Fix undo/redo history when inserting a link configured to open in a new tab #50460

Merged
merged 14 commits into from
May 24, 2023

Conversation

derekblank
Copy link
Member

@derekblank derekblank commented May 9, 2023

What?

Fixes a bug affecting undo/redo functionality on mobile. When a link is configured to open in a new tab and text is typed after this link, it is not possible to undo and then use the Redo button to redo the text that was added after the link.

Original issue:

Related issues:

Gutenberg Mobile PR:

Why?

There are several related issues that describe the broken behavior with undo/redo when using the "Open in new tab" option on mobile. One symptom of this issue is that an extra state event occurs on mobile when a link is set to open in a new tab. This is demonstrated visually in wordpress-mobile/gutenberg-mobile#2091 (comment) with further information. Configuring a link to open in a new tab shifts the rel attribute within the HTML, which causes the edit reducer to disrupt the redo history.

Example HTML when a link is created without opening in a new tab:

<a href="https://wordpress.com" target="_blank" rel="noreferrer noopener">WordPress.com</a>

Example HTML when a link is created and configured to open in a new tab:

<a rel="noreferrer noopener" href="https://wordpress.com" target="_blank">WordPress.com</a>

How?

This PR adds rel to the object of attributes passed to the link formatting, along with url and target:

attributes: {
	url: 'href',
	target: 'target',
	rel: 'rel',
},

When rel is passed with the attributes object to registerFormatType, the rel attribute is no longer an unassigned attribute, and does not shift places when the HTML attributes are re-serialized. Therefore, the extra state event is not fired on mobile. On the web, the extra state event is not fired in either circumstance, and the re-serialization of the attributes has no effect on behavior.

Testing Instructions

  1. In a post, add a paragraph block and type some text.
  2. Create a link using the Link button in the Bottom Bar.
  3. When configuring the link options, toggle the switch to Open in new tab.
  4. Switch to HTML mode, and note the position of the rel attribute within the link's HTML. (It should be the final attribute.)
  5. Switch back to Visual mode and continue typing a few more words of text.
  6. Test the Undo and Redo buttons in the Bottom Bar.
  7. Use the Undo button and step backward to at least the point before the link was created.
  8. Use the Redo button to step forward to the point where the link was created, and also step forward to the text that was typed after the link.

Demo builds for testing:

Screenshots

Undo-Redo.mov

@derekblank derekblank added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Mobile App - Automation Label used to initiate Mobile App PR Automation labels May 9, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

Size Change: 0 B

Total Size: 1.4 MB

ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.33 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 472 B
build/block-directory/index.min.js 7.18 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/content-rtl.css 4.22 kB
build/block-editor/content.css 4.21 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 B
build/block-editor/index.min.js 198 kB
build/block-editor/style-rtl.css 15.1 kB
build/block-editor/style.css 15.1 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 91 B
build/block-library/blocks/avatar/style.css 91 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 584 B
build/block-library/blocks/button/editor.css 582 B
build/block-library/blocks/button/style-rtl.css 624 B
build/block-library/blocks/button/style.css 623 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 409 B
build/block-library/blocks/columns/style.css 409 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.61 kB
build/block-library/blocks/cover/style.css 1.6 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 159 B
build/block-library/blocks/details/style.css 159 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/interactivity.min.js 395 B
build/block-library/blocks/file/style-rtl.css 269 B
build/block-library/blocks/file/style.css 270 B
build/block-library/blocks/file/view.min.js 375 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/freeform/style-rtl.css 246 B
build/block-library/blocks/freeform/style.css 246 B
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 336 B
build/block-library/blocks/html/editor.css 337 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/style-rtl.css 652 B
build/block-library/blocks/image/style.css 652 B
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 712 B
build/block-library/blocks/navigation-link/editor.css 711 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.33 kB
build/block-library/blocks/navigation/editor.css 2.33 kB
build/block-library/blocks/navigation/interactivity.min.js 865 B
build/block-library/blocks/navigation/style-rtl.css 2.21 kB
build/block-library/blocks/navigation/style.css 2.2 kB
build/block-library/blocks/navigation/view-modal.min.js 2.81 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 319 B
build/block-library/blocks/post-featured-image/style.css 319 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 281 B
build/block-library/blocks/post-template/style.css 281 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 450 B
build/block-library/blocks/query/editor.css 449 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 178 B
build/block-library/blocks/search/editor.css 178 B
build/block-library/blocks/search/style-rtl.css 434 B
build/block-library/blocks/search/style.css 432 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 348 B
build/block-library/blocks/spacer/editor.css 348 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 433 B
build/block-library/blocks/table/editor.css 433 B
build/block-library/blocks/table/style-rtl.css 645 B
build/block-library/blocks/table/style.css 644 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12 kB
build/block-library/editor.css 12 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 204 kB
build/block-library/interactivity/runtime.min.js 2.23 kB
build/block-library/interactivity/vendors.min.js 8.15 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.9 kB
build/block-library/style.css 12.9 kB
build/block-library/theme-rtl.css 686 B
build/block-library/theme.css 691 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 50.9 kB
build/commands/index.min.js 15 kB
build/commands/style-rtl.css 827 B
build/commands/style.css 827 B
build/components/index.min.js 232 kB
build/components/style-rtl.css 11.7 kB
build/components/style.css 11.7 kB
build/compose/index.min.js 12.4 kB
build/core-commands/index.min.js 1.8 kB
build/core-data/index.min.js 16.5 kB
build/customize-widgets/index.min.js 12.2 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 708 B
build/data/index.min.js 8.68 kB
build/date/index.min.js 40.5 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.72 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/index.min.js 35.2 kB
build/edit-post/style-rtl.css 7.76 kB
build/edit-post/style.css 7.75 kB
build/edit-site/index.min.js 63.9 kB
build/edit-site/style-rtl.css 10.5 kB
build/edit-site/style.css 10.5 kB
build/edit-widgets/index.min.js 17.3 kB
build/edit-widgets/style-rtl.css 4.53 kB
build/edit-widgets/style.css 4.53 kB
build/editor/index.min.js 45.8 kB
build/editor/style-rtl.css 3.54 kB
build/editor/style.css 3.53 kB
build/element/index.min.js 4.89 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.77 kB
build/format-library/style-rtl.css 554 B
build/format-library/style.css 553 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.73 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.91 kB
build/list-reusable-blocks/index.min.js 2.14 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.97 kB
build/notices/index.min.js 963 B
build/plugins/index.min.js 1.85 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.33 kB
build/primitives/index.min.js 944 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 939 B
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.74 kB
build/reusable-blocks/index.min.js 2.25 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/rich-text/index.min.js 11 kB
build/router/index.min.js 1.78 kB
build/server-side-render/index.min.js 2.08 kB
build/shortcode/index.min.js 1.42 kB
build/style-engine/index.min.js 1.52 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.65 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 268 B
build/widgets/index.min.js 7.28 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@derekblank derekblank marked this pull request as ready for review May 10, 2023 00:26
@derekblank derekblank changed the title [RNMobile] Fix undo/redo [RNMobile] Fix undo/redohistory when inserting a link configured to open in a new tab May 10, 2023
@github-actions
Copy link

github-actions bot commented May 10, 2023

Flaky tests detected in c4bb30003698367981d4bcc837f9a796f9d7f523.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5055066858
📝 Reported issues:

@derekblank derekblank changed the title [RNMobile] Fix undo/redohistory when inserting a link configured to open in a new tab [RNMobile] Fix undo/redo history when inserting a link configured to open in a new tab May 10, 2023
Co-authored-by: Tanner Stokes <tanner.stokes@automattic.com>
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

Super interesting! Thank you for researching and addressing this.

The prototype iOS build does not pass the test plan. However, my guess is this occurs because the prototype build does not include the fixed code. The wordpress-mobile/gutenberg-mobile#5747 PR does not include a new bundle via npm run bundle. The Android prototype build succeeds, though. 👍🏻

Would you be willing to follow up with tests covering this bug? In general, I think it would be good to always include a test with each bug we fix. For this specific bug, adding a test to the Editor History file makes sense to me.

packages/react-native-editor/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: David Calhoun <github@davidcalhoun.me>
@derekblank
Copy link
Member Author

The wordpress-mobile/gutenberg-mobile#5747 PR does not include a new bundle via npm run bundle.

Thanks for pointing this out. There were a handful of unrelated CI issues on the Gutenberg Mobile PR, and I ended up manually updating the Gutenberg ref (possibly incorrectly), which may have caused it the branches to be out of sync. I'll update the iOS build. 👍

Would you be willing to follow up with tests covering this bug? In general, I think it would be good to always include a test with each bug we fix. For this specific bug, adding a test to the Editor History file makes sense to me.

This is a good idea, and I'll start working on it to include before we merge this PR.

@dcalhoun
Copy link
Member

The wordpress-mobile/gutenberg-mobile#5747 PR does not include a new bundle via npm run bundle.

Thanks for pointing this out. There were a handful of unrelated CI issues on the Gutenberg Mobile PR, and I ended up manually updating the Gutenberg ref (possibly incorrectly), which may have caused it the branches to be out of sync. I'll update the iOS build. 👍

You may already understand this, but your response gave me pause. The updated Gutenberg ref (git submodule commit SHA) is updated correctly. The missing piece is committing an updated app bundle produced via npm run bundle within gutenberg-mobile. A Buildkite CI tasks publishes an artifact for Android, but there is no such CI task for iOS. In order to integrate an updated Gutenberg ref into a WordPress-iOS prototype build, we must run npm run bundle and commit the modified files.

@derekblank
Copy link
Member Author

The missing piece is committing an updated app bundle produced via npm run bundle within gutenberg-mobile. A Buildkite CI tasks publishes an artifact for Android, but there is no such CI task for iOS. In order to integrate an updated Gutenberg ref into a WordPress-iOS prototype build, we must run npm run bundle and commit the modified files.

No, I was actually not aware of that, but that makes much more sense now. I was aware of the npm run bundle command, but have only used it in the context of managing a release. Specifically, I didn't put together that there isn't a matching CI task for iOS. This is the first time I've needed/wanted to create a prototype build outside of creating a release, so thanks again for clarifying that.

I was looking for documentation on this, and noted that it is mentioned in the WPiOS Integration doc. I had only read about it in the release context over in the release-toolkit-gutenberg-mobile docs. I test PRs in the host apps using the local gutenberg-mobile checkout, so it's possible that I once read the latter half of the WPiOS Integration doc and put it out of my mind. Do you think it would be beneficial to link or mention the host app Integration documentation from the Gutenberg Mobile README?

@dcalhoun
Copy link
Member

Do you think it would be beneficial to link or mention the host app Integration documentation from the Gutenberg Mobile README?

Yeah, sounds like a good addition to the existing "Run" section. I think it makes sense for the README to default to the Demo editor, but a note could be added linking to documentation on how to integrate into the host WordPress iOS and Android apps.

derekblank and others added 7 commits May 22, 2023 16:43
The targeted elements do not require asynchronous queries. Using
synchronous queries is more concise and easier to comprehend.
The targeted bug occurs when:

1. Changing a link to "Open in a new tab."
2. Performing additional changes, e.g. typing.
3. Undoing both actions.
4. Redoing both actions.

Thus, this changes the initial HTML, adds additional undo/redo
invocations, and updates inline snapshots to match the expected
outcomes.
@derekblank
Copy link
Member Author

@dcalhoun I've added a WIP for a Editor History E2E test. I'm curious of your thoughts on this approach before proceeding further. Although just a scaffold at the moment, the test structure is similar to the integration test in its arrangements and actions but with more realistic editor behavior, and could potentially replace the integration test in purpose.

Specifically:

  • Test location. I placed the Editor History E2E along with other E2E tests in the react-native-editor package, as opposed to placing it within __device-tests__ in the root of Gutenberg Mobile. The Gutenberg Mobile E2E tests felt more snapshot-focused, while the react-native-editor E2E tests felt more assertion focused. I'm trying to understand if there is a more nuanced difference than that between the two test locations. The current test strategy I have outlined for Editor History E2E feels more assertion focused, but I am open to another perspective.
  • Test separation of concerns. I've created a separate file for the Editor History E2E test. Would it make more sense to include as a test case in an existing E2E file, like gutenberg-editor-paragraph.test.js? Since it is intended to test editor history, I think it makes sense as its own file.
  • Selectors and helpers. In the new Editor History E2E test file, I've left scaffolding comments where new E2E helpers and selectors would need to be implemented. If you have a moment to review, I'd be interested in any tips or caveats on these scaffolded areas -- particularly if you are able to point to any helpers that may already exist that would accomplish the described action (e.g., selecting the text input from a paragraph block, populating it with text, selecting a specific word of that text to "highlight" in order to press the Link button and invoke the Link Settings bottom sheet, etc. I believe most of these helpers/selectors will need to be created.)

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

The Gutenberg Mobile E2E tests felt more snapshot-focused, while the react-native-editor E2E tests felt more assertion focused. I'm trying to understand if there is a more nuanced difference than that between the two test locations.

The tests in the former location are either Automattic-centric (e.g. Jetpack) or experimental (e.g. visual snapshots). Additionally, you may note that not every test in latter location are run within the gutenberg CI; this was done due to how slow and flaky the suite is. Ideally, we would run all relevant tests within the gutenberg CI. Efforts to replace e2e tests with integration tests relate to this subject.

Also, not every e2e test is automatically run for every PR within gutenberg-mobile. You will note the "canary" name in certain e2e test files. These are run within the gutenberg-mobile CI for every PR. All other e2e tests are run when a person manually initiates the "Optional UI Tests" CI task in CircleCI.

We are careful to add e2e tests that run by default in gutenberg-mobile and even more so within gutenberg. Placing this in gutenberg makes sense to me, but we should likely not run it in the CI.

Would it make more sense to include as a test case in an existing E2E file, like gutenberg-editor-paragraph.test.js? Since it is intended to test editor history, I think it makes sense as its own file.

I think this is largely a judgement call. It makes sense from a subject matter perspective that a single file focuses upon the feature of editor history. However, there is a test environment setup cost we pay for each test file. I.e. because e2e tests are so slow, it is beneficial to group several together rather than creating a new environment for each individual test.

If you have a moment to review, I'd be interested in any tips or caveats on these scaffolded areas -- particularly if you are able to point to any helpers that may already exist that would accomplish the described action

I left inline comments with any suggestions I have. However, keep in mind there are a lot of helpers and I am not intimately familiar with them all. I may have missed opportunities. My technique is to "fold" all code in the editor-page and utils files to get a high-level view of the available helpers.

Folded test helper code

folded-test-helper-code

Additionally, my recommendation would be to postpone creating new test helpers until we identify common patterns across several tests or a need to simplify a very complex or brittle interaction. As discussed, there are a lot of helpers; frequently adding more will continue to increase the incomprehensibility of them, reducing the usefulness of the helpers.

Lastly — and I should have shared this in my original review — if we believe completing new test coverage will take a good amount of time, it may be worth merging this PR without tests and opening a separate PR adding tests. That might allow us to deliver a bug fix to users sooner.

@derekblank derekblank force-pushed the rnmobile/fix/undo-redo-history branch from c4bb300 to d0098e6 Compare May 23, 2023 23:58
@derekblank
Copy link
Member Author

derekblank commented May 24, 2023

@dcalhoun Thank you for reviewing, and for your explanations regarding the differences between the Gutenberg and Gutenberg Mobile CI tests, as well as the explanations for CI considerations. That all makes sense. 👍

Lastly — and I should have shared this in my original review — if we believe completing new test coverage will take a good amount of time, it may be worth merging this PR without tests and opening a separate PR adding tests. That might allow us to deliver a bug fix to users sooner.

I agree here too. After also discussing with @twstokes, I think we should pause on the E2E test due to complexities of writing additional helpers, and potentially include these at a later time.

The integration test is actually correctly checking for the shifting rel attribute, which is the symptom of the issue, meaning the test fails when the actual fix is removed. Due to that, I think the integration test is worth retaining as it does help to identify the issue, and is not expensive to run. Let me know if you have any further thoughts about including (or excluding) the integration test before we merge. 🙇

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

Thank you for adding the test coverage. 🙇🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants