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

Block Editor: Fix Multiple Trailing Inserters for Nested Inner Blocks #24836

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Aug 27, 2020

Note:

This is a potential fix. It seems like this issue has been discussed in the past, and there might be UX reasoning behind why we want to show duplicate trialing inserters. I think it's worthwhile to discuss, but for now, I drafted a PR to frame our conversation in something concrete. I will be posting a quick summary of alternative ways we can proceed here #24360

Description

Blocks, like the group or query loop block, render what are called inner blocks. These inner blocks use block list blocks to display ordered content. The trailing inserter is included as the last item in block lists.

When blocks with inner blocks are nested in other inner blocks, the trailing inserter is rendered multiple times. For example, if a query loop block is placed within a group block, both will render their own set of inner blocks, which subsequently display two trailing inserters.

One potential solution is to display a single trailing inserter at any given time. We determine which inserter to show by evaluating the currently selected block and ensuring that the trailing inserter is a sibling (that way we're certain they're children of the same inner block)

How has this been tested?

  1. Enable site editing locally
  2. Navigate to the site editor
  3. Click on a child of a set of inner blocks, where inner blocks are the content for group blocks, query loop blocks, etc.)
  4. Verify that multiple trailing inserters are not displayed at any given time
  5. Verify the same behavior in the post editor

Screenshots

Before

Bad Trailing Inserter

After

Trailing Inserter

Types of changes

Potential bug fix for #24360

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.

@jeyip jeyip added Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor labels Aug 27, 2020
@jeyip jeyip self-assigned this Aug 27, 2020
@noahtallen
Copy link
Member

I like the direction this goes! It's a plus one from me, because I think showing multiple block inserters at the same level is confusing in this situation. Particularly because the two appearing close together (from the group block and from the template part block) don't seem like they would insert a block into separate entities, but they actually do.

@github-actions
Copy link

github-actions bot commented Aug 27, 2020

Size Change: +51 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 128 kB +51 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.57 kB 0 B
build/block-library/editor.css 8.57 kB 0 B
build/block-library/index.js 137 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 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 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 305 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.25 kB 0 B
build/edit-site/index.js 17.1 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 12 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.6 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 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.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 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/media-utils/index.js 5.32 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 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 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 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@jeyip
Copy link
Contributor Author

jeyip commented Aug 27, 2020

Note:

I noticed interesting behavior, which seems intended, while testing this fix. The trailing inserter is only shown if the last block in a set of inner blocks is not a paragraph. If you're not seeing the trailing inserter when you'd expect it to display, add any block besides a paragraph block to the last position of a group block, query loop block, etc.

If you're curious about technical details, the execution thread leads here to these lines of code, which simply check if the name of the last block is core/paragraph. If it returns true, the trailing inserter is not rendered.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

This behavior seems to be working very well! I like it and think it is an improvement. For example if we have Group B nested in Group A, then selecting Group B will only add the appender to the end of Group B (as opposed to adding one for each nested group). It seems like a good change because having multiple inserters show up on selection is disorienting, confusing, and unnecessary.

I noticed interesting behavior, which seems intended, while testing this fix. The trailing inserter is only shown if the last block in a set of inner blocks is not a paragraph.

Yeah this seems to be pre-existing behavior, but I don't think it completely working as intended? As soon as you click the appender it becomes an empty paragraph block by default and it would be odd for another appender to pop beneath that as soon as you clicked on it, so this part seems intended. However, the way it is currently written means that appender will never appear after any paragraph block (even if it is pre-existing and full of content).

I would think that condition that checks if lastBlock === core/paragraph should be expanded to also require that that last paragraph block has no actual content 🤔 (or the content starts with '/' and has no spaces so an appender isnt added when a block is being initialized via "/blockName" shortcut). @youknowriad - do you happen to know anything about why it behaves this way?

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 27, 2020

The special-case behavior for Paragraph blocks definitely was intended at some point, but I suspect time may have changed a lot of people's opinions on it. (I recall how lots of people were on board for increasing block padding for parent blocks to make them easier to select... only for a lot of the same people to change their minds a few months later after better solutions were found and implemented. Similarly, I recall how the decision to remove the quick-insert shortcuts was almost comically unanimous.)

Personally, I've come to think the no-appender behavior with Paragraph blocks is weird and inconsistent, since even other text-related blocks lack the behavior. I think we ought to make the behavior consistent across all blocks.

Anyway, as for this PR, I like the idea behind it. I think it's a definite improvement over master, keeping in the spirit of only showing controls contextually and looking like the front-end as much as possible.

I did catch one issue, though. Check out empty Column blocks:

image

@Addison-Stavlo
Copy link
Contributor

I did catch one issue, though. Check out empty Column blocks:

Ah, yes im seeing this as well. Actually inserting the columns block is a bit uglier on my end, it ends up removing that block appender and the selected column has no height:

Screen Shot 2020-08-26 at 11 14 49 PM

Where normally we would expect:
Screen Shot 2020-08-26 at 11 15 25 PM

So this seems to be effecting more than just the sibling appender 🤔

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Aug 27, 2020

@Addison-Stavlo This PR was never modifying the sibling inserter in the first place. It's modifying the "trailing appender" (or whatever it's called). As it turns out, the Column block placeholders are just restyled trailing appenders, hence why they're affected.

@jasmussen
Copy link
Contributor

Thanks very much for tackling this head on! The issue with multiple inserters is especially frustrating in patterns where you might see a group with a background, a columns inside, then an image, and you'll end up with 3 black plus buttons. This is an important problem to solve, as in many cases it actuallly breaks the layout and definitely visually misleads.

The precise behavior is hard to get right, though. The primary challenge the trailing appender is meant to solve is the situation where a non-text block is the last block in a container, such as an image. While you can always select an image and press Enter when it has focus and it creates a new paragraph beneath, this in practice has not been intuitive to folks. Importantly, that issue appears to be addressed in this PR, and generally I would focus the most testing on ensuring that aspect is handled well. For that case, the principle "only show one at a time" seems right.

In superficial testing, the heuristic doesn't seem perfect yet for an empty post. This seems a bit bare:

Screenshot 2020-08-27 at 08 58 29

Blocks, like the group or  template part block, render what are called inner blocks. These inner blocks use block list blocks to display ordered content. The trailing inserter is included as the last item in block lists.

When blocks with inner blocks are nested in other inner blocks, the trailing inserter is rendered multiple times. For example, if a template part block is rendered within  a group block, each renders its own inner blocks, which subsequently render two trailing inserters.

One potential solution is to display a single trailing inserter at any given time. We determine which inserter to show by evaluating the currently selected block and ensuring that the trailing inserter is a sibling (that way we're certain they're children of the same inner block)
@jeyip jeyip force-pushed the try/displaying-one-trailing-inserter-at-a-time branch 3 times, most recently from fdf1080 to ee9d41d Compare August 27, 2020 23:58
@jeyip jeyip force-pushed the try/displaying-one-trailing-inserter-at-a-time branch from ee9d41d to 97d9cea Compare August 28, 2020 04:34
@jeyip
Copy link
Contributor Author

jeyip commented Aug 28, 2020

@jasmussen @ZebulanStanphill @Addison-Stavlo

Thanks for the feedback! The columns block and empty post state should be fixed.

An important change to note is that, with the columns block, only one trailing inserter is displayed at any given time. Given the modifications we're making in this PR, this is expected, but it definitely changes the columns block experience. I'm not sure if anyone feels strongly about UX updates.

Before

Screen Shot 2020-08-27 at 9 43 35 PM

After

Screen Shot 2020-08-27 at 9 43 49 PM

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Aug 28, 2020

Nice updates, I like how this is taking shape. However, I am noticing another side effect here. Previously, when we had the Column itself selected we were given the appender at the bottom of that column. Now with the column selected there doesn't seem to be an appender for it, we must select a block in that column in order to get the appender to appear:

Before
Screen Shot 2020-08-28 at 1 25 00 PM

After
Screen Shot 2020-08-28 at 1 26 00 PM

But still appears when the block in the column is selected:
Screen Shot 2020-08-28 at 1 27 27 PM


Regarding the 3 trailing appenders (one for each column) - This is actually behaving oddly for me on master. I reproduced it with the 3 appender once, but am unable to reproduce it anymore. A lot of the time I am just seeing the appender for just the one column that is selected anyways. Even chalking this up to my own local env weirdness, and assuming that we always currently have the 3 appenders on master, I feel only showing the appender for the selected column is not a bad change.

@jeyip
Copy link
Contributor Author

jeyip commented Aug 28, 2020

However, I am noticing another side effect here. Previously, when we had the Column itself selected we were given the appender at the bottom of that column. Now with the column selected there doesn't seem to be an appender for it, we must select a block in that column in order to get the appender to appear

@Addison-Stavlo This is a good observation 👍. I wanted to note that this behavior is expected in the sense that, in our code, we've outlined logic to only display the trailing inserter for one set of inner blocks at any given time.

To elaborate, clicking on an inner block for one column, like a Site Logo block, displays the trailing inserter for column inner blocks. Clicking on an inner block for column(s), however, like a column block, should display a different trailing inserter. It's empty because there is no trailing inserter for columns inner blocks, which is also behavior that exists on master.

Click on column inner block Clicking on columns inner block
Screen Shot 2020-08-28 at 1 27 27 PM Screen Shot 2020-08-28 at 1 26 00 PM

With all of that being said, I do agree that the "Before" / previous behavior seems more ideal for user interactions. I personally expect to see a trailing inserter for column inner blocks, regardless of whether or not I click on the column block or a column inner block. I only wanted to call this out because the change to address this would be more of an exception to our new business logic for trailing inserters, and less of a "fix" for aberrant behavior.

Hopefully that makes sense. I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

@noahtallen
Copy link
Member

Hopefully that makes sense. I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

Are there other blocks to which this could apply, or maybe a more general way to improve this? I guess, what exactly makes the columns block special in this situation?

@Addison-Stavlo
Copy link
Contributor

Hopefully that makes sense.

Yeah, that makes sense!

I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

It seems to be a change to group blocks as well. Previously with the group block selected the appender was added to the bottom inside the group block, similarly now it is not. Im not sure what the best approach is here, but creating case by case exceptions may not be the best way to go about it. 🤔 Maybe the extra qualifier should be that if it is a block that can contain inner blocks it will still need the appender inside of it? May need more external input here...

@jeyip
Copy link
Contributor Author

jeyip commented Aug 28, 2020

It seems to be a change to group blocks as well. Previously with the group block selected the appender was added to the bottom inside the group block, similarly now it is not.

Oh good catch. This is going to happen with any block that implements inner blocks. An inserter should still be displayed alongside the group block, but not for its content. @Addison-Stavlo

Screen Shot 2020-08-28 at 2 13 22 PM

Maybe the extra qualifier should be that if it is a block that can contain inner blocks it will still need the appender inside of it?

This would reintroduce the look of "duplicated" inserters. Imagine a nesting like group 1 > group 2 > paragraph. Group 1 and Group 2 can both have inner blocks. With that logic, an inserter to place blocks in group 1 will always be displayed alongside the group 2 block because it will have the extra qualifier. Another inserter to place blocks in group 2 will be displayed alongside the paragraph block because it will also have the extra qualifier.

maybe a more general way to improve this?

I'm going to give the business logic more thought. My intuition says that it might not be possible with our current approach.

@ZebulanStanphill ZebulanStanphill added the Needs Accessibility Feedback Need input from accessibility label Aug 28, 2020
@jeyip jeyip force-pushed the try/displaying-one-trailing-inserter-at-a-time branch from 32e48e3 to f12cb55 Compare August 28, 2020 22:44
@jeyip
Copy link
Contributor Author

jeyip commented Aug 28, 2020

@Addison-Stavlo I pushed changes to support the use case where we want the inserter block displayed when a parent is selected. It should address the issues you brought up with the group and column blocks.

It reintroduces some instances where we see "duplicate" inserters, specifically when we select a block with inner blocks nested within another block. (Ex. Group 1 > Group 2 > Paragraph, where we select Group 2) I still think that overall, it's an improvement over the behavior we were struggling with before.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

It should address the issues you brought up with the group and column blocks.
It reintroduces some instances where we see "duplicate" inserters

Hrm, testing seems to reintroduce the duplicates as they were without fixing the parent selection.. but I think its just a small slip in the logic (listed below).

With that change, you are right... it does fix the issue I noted with the parent selected. It reintroduces the duplicate in some cases as you mentioned, but I agree it is still better behavior than before!

This is tricky, but it is starting to look pretty good. 😁

Co-authored-by: Addison Stavlo <Stavz01@gmail.com>
@jeyip
Copy link
Contributor Author

jeyip commented Aug 31, 2020

Everything looks to be in a good place. This PR is ready for another round of CR and feedback @ZebulanStanphill @MichaelArestad @noahtallen @Addison-Stavlo 🙂

Thanks for the help everyone!

@noahtallen
Copy link
Member

I'm not exactly sure which edge cases to test for here, so I decided to give it a spin and see how it felt. Unfortunately, I don't think this fixes the use cases I was expecting it to fix!

This is on a fresh local install. I've made no edits: just selected to the left of where the top template part is so that it would select the blocks there.

Screen Shot 2020-09-02 at 3 13 24 PM

I think the block structure in this theme is:

- group
  - template part
    - group (this is the one selected in the screenshot above

@jeyip
Copy link
Contributor Author

jeyip commented Sep 3, 2020

I'm not exactly sure which edge cases to test for here, so I decided to give it a spin and see how it felt.

@noahtallen I can clarify what we can expect from this PR now:

When we select blocks with no inner blocks, there should no longer be duplicate inserters. (Ex. Selecting Paragraph from Group > Template Part > Paragraph)

Screen Shot 2020-09-02 at 5 54 58 PM


  1. When there are sets of nested inner blocks and we select the outermost parent, there should no longer be duplicate inserters (Ex. Selecting Group from Group > Template Part > Paragraph)

Screen Shot 2020-09-02 at 5 55 25 PM


  1. A trailing inserter for the root document will always be shown as it is today.

Screen Shot 2020-09-02 at 5 58 24 PM

@jeyip
Copy link
Contributor Author

jeyip commented Sep 3, 2020

Seeing duplicated inserters when selecting a block (with inner blocks), nested in another set of inner blocks, was previously discussed here and here. There are certain blocks (like the column and group block) where we'd like to display the trailing inserter when:

  1. The block itself is selected
  2. The block's children are selected

This leads to the issue you're seeing. When we have a series of nested inner blocks, we run into the situation where we display 1 inserter for the selected block (because it's selected # 1) and 1 inserter for its parent (because the parent's child is selected # 2). It's an unfortunate tradeoff, and if this doesn't quite improve the template part editing experience enough, maybe we could explore your suggestion here @noahtallen ?

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Block structure is Group -> Template Part -> paragraph

  • ✅ Only one inserter when group is selected (for the group block)
  • ❓ Double inserters when template part is selected for both group and template part.
  • ✅ Only one inserter when the paragraph block is selected (for the template part block).

I think the second scenario is the exception you described? I'm not super happy with it here, but I can definitely accept that it is necessary for supporting other blocks. I do wish we could be a bit smarter about it.

That said, from my perspective looking at the template parts, this is an improvement in several cases, so I'll give it a +1! 🚀

@jeyip
Copy link
Contributor Author

jeyip commented Sep 3, 2020

I'm not super happy with it here, but I can definitely accept that it is necessary for supporting other blocks. I do wish we could be a bit smarter about it.

It definitely felt like we could be smarter about it. I spent some time last night thinking about a few last changes we could make to improve the experience. Long story short, I add a condition that checks for nested inner blocks. It should fix the issues you were seeing earlier, Noah. Not sure if it's perfect, but it's an easy revert, and I think it noticeably improves the editing experience. Let me know what you think @noahtallen @Addison-Stavlo

@noahtallen
Copy link
Member

Hm. I think we might want to play it safe and not include that change for now. It is more difficult to add blocks in scenarios where you have column -> group. You can't add another block to the column unless you select the column itself in this scenario, which is very difficult if you aren't comfortable with "select parent block" or the breadcrumb.

But I do really like it for the template part use cases we are looking at :D

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 Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants