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

Fix issue where block inserted in wrong place when selection is in nested block list, but root appender is used. #23668

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 3, 2020

Description

Fixes #23263. This also seems to fix an issue where the trailing inserter is sometimes on the left and sometimes on the right.

When clicking the trailing root appender while selection is in an inner blocks list, the block will be inserted where selection is instead of at the root level.

I did some digging and it looks like the code was added in #22140 for the widgets screen, so this PR essentially reverted part of that. This change might break things there, but it's hard to tell what.

How has this been tested?

  1. Start a new post
  2. Add a buttons block
  3. Make sure a button block in the buttons block is selected
  4. Click the '+' appender that appears at the end of the post
  5. Notice a button is inserted within the buttons block

Screenshots

Screenshot 2020-06-18 at 3 47 04 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Jul 3, 2020
@talldan talldan self-assigned this Jul 3, 2020
@talldan talldan changed the title Fix blocks inserted in wrong place when selection is in nested block list, but root appender is used. Fix issue where block inserted in wrong place when selection is in nested block list, but root appender is used. Jul 3, 2020
} = select( 'core/block-editor' );
const { getBlockVariations } = select( 'core/blocks' );

rootClientId =
rootClientId ||
getBlockRootClientId( clientId || getBlockSelectionEnd() ) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was that rootClientId and clientId are undefined for the root appender, so this fallback to getBlockSelectionEnd would result in blocks being inserted in the wrong place (and potentially the wrong inserter items displayed).

@github-actions
Copy link

github-actions bot commented Jul 3, 2020

Size Change: +1.79 kB (0%)

Total Size: 1.13 MB

Filename Size Change
build/annotations/index.js 3.68 kB +53 B (1%)
build/autop/index.js 2.82 kB +1 B
build/block-directory/index.js 7.67 kB +512 B (6%) 🔍
build/block-directory/style-rtl.css 944 B -8 B (0%)
build/block-directory/style.css 945 B -6 B (0%)
build/block-editor/index.js 110 kB +1.22 kB (1%)
build/block-editor/style-rtl.css 10.7 kB -36 B (0%)
build/block-editor/style.css 10.7 kB -36 B (0%)
build/block-library/editor-rtl.css 7.57 kB -62 B (0%)
build/block-library/editor.css 7.57 kB -64 B (0%)
build/block-library/index.js 130 kB +246 B (0%)
build/block-library/style-rtl.css 7.78 kB -7 B (0%)
build/block-library/style.css 7.79 kB -5 B (0%)
build/block-library/theme-rtl.css 728 B -2 B (0%)
build/block-library/theme.css 729 B -3 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB -1 B
build/block-serialization-spec-parser/index.js 3.1 kB -5 B (0%)
build/blocks/index.js 48.2 kB +71 B (0%)
build/components/index.js 199 kB +74 B (0%)
build/components/style-rtl.css 15.8 kB -29 B (0%)
build/components/style.css 15.8 kB -27 B (0%)
build/compose/index.js 9.56 kB -89 B (0%)
build/data-controls/index.js 1.29 kB +2 B (0%)
build/data/index.js 8.44 kB +1 B
build/date/index.js 5.38 kB -89 B (1%)
build/dom/index.js 3.23 kB +38 B (1%)
build/edit-navigation/index.js 9.93 kB -47 B (0%)
build/edit-navigation/style-rtl.css 1.02 kB -1 B
build/edit-navigation/style.css 1.02 kB -1 B
build/edit-post/index.js 304 kB +136 B (0%)
build/edit-post/style-rtl.css 5.57 kB +8 B (0%)
build/edit-post/style.css 5.57 kB +8 B (0%)
build/edit-site/index.js 16.6 kB -88 B (0%)
build/edit-site/style-rtl.css 3.03 kB +2 B (0%)
build/edit-site/style.css 3.03 kB +2 B (0%)
build/edit-widgets/index.js 9.35 kB +25 B (0%)
build/edit-widgets/style.css 2.45 kB -1 B
build/editor/index.js 45 kB +193 B (0%)
build/editor/style-rtl.css 3.78 kB -37 B (0%)
build/editor/style.css 3.78 kB -40 B (1%)
build/element/index.js 4.65 kB +1 B
build/format-library/index.js 7.73 kB -1 B
build/hooks/index.js 2.13 kB +2 B (0%)
build/keyboard-shortcuts/index.js 2.52 kB +5 B (0%)
build/keycodes/index.js 1.94 kB +2 B (0%)
build/media-utils/index.js 5.32 kB +22 B (0%)
build/notices/index.js 1.79 kB +4 B (0%)
build/nux/index.js 3.41 kB +2 B (0%)
build/primitives/index.js 1.41 kB -92 B (6%)
build/rich-text/index.js 13.9 kB -95 B (0%)
build/server-side-render/index.js 2.71 kB +37 B (1%)
build/viewport/index.js 1.85 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 620 B 0 B
build/core-data/index.js 11.4 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 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 710 B 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ellatrix
Copy link
Member

ellatrix commented Jul 3, 2020

Would it be possible to add an e2e test?

@talldan talldan requested review from ajitbohra, nerrad and ntwb as code owners July 7, 2020 08:03
@talldan
Copy link
Contributor Author

talldan commented Jul 7, 2020

@ellatrix E2E tests now added.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@talldan talldan merged commit 69643b0 into master Jul 8, 2020
@talldan talldan deleted the fix/blocks-inserted-in-wrong-place branch July 8, 2020 01:10
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 8, 2020
@talldan talldan added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 8, 2020
youknowriad pushed a commit that referenced this pull request Jul 13, 2020
…sted block list, but root appender is used. (#23668)

* Remove use of getSelectionEnd to determine where blocks should be inserted

* Add e2e tests
@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 13, 2020
@youknowriad
Copy link
Contributor

@talldan Looks like this PR broke the global inserter in the widgets screen. I'm not sure what's the best path forward to avoid both issues in both places.

@youknowriad
Copy link
Contributor

Fix here #24045

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing appender/inserter inserts blocks in the wrong place when Button or Navigation Link are selected
3 participants