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

Make sure we don't search the directory for child items. #24148

Closed

Conversation

StevenDufresne
Copy link
Contributor

@StevenDufresne StevenDufresne commented Jul 23, 2020

Description

This PR makes sure that we don't search the block directory when we are displaying the child blocks otherwise the block inserter does not recognize that the block was already installed.

How to replicate the issue?

  1. Search for 'Cornell'
  2. Install "Cornell Note" block
  3. Notice that the block installs properly by the block directory result remains:

screenshot of broken state

Why is this happening?

This problem occurs because the block enforces children blocks and this leads to 2 problems:

  1. The rootClientId changes emptying filteredItems and therefore showing the block directory component.
  2. The filter value doesn't match any child blocks and therefore they don't show up.

Types of changes

  1. Pass hasChildItems& hasAllowedBlocks into the slot/fill and return null in the Block Directory package plugin component should either match.

Question/Improvements

After the block installs, we move the focus into the block. Since the filterValue doesn't match the name of the child block, the user sees no results in the inserter.

You can view the behaviour here (with the fix to stop searching the block directory): https://d.pr/v/QDVQ0p.

Should we clear the filter value for blocks with children blocks?

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.

@StevenDufresne StevenDufresne added [Type] Bug An existing feature does not function as intended [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Jul 23, 2020
@github-actions
Copy link

github-actions bot commented Jul 23, 2020

Size Change: +52 B (0%)

Total Size: 1.18 MB

Filename Size Change
build/block-directory/index.js 8.57 kB +20 B (0%)
build/block-editor/index.js 129 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 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 8.6 kB 0 B
build/block-library/editor.css 8.6 kB 0 B
build/block-library/index.js 135 kB 0 B
build/block-library/style-rtl.css 7.66 kB 0 B
build/block-library/style.css 7.65 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.6 kB 0 B
build/date/index.js 31.9 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.42 kB 0 B
build/edit-navigation/index.js 10.7 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/edit-site/index.js 20.4 kB 0 B
build/edit-site/style-rtl.css 3.84 kB 0 B
build/edit-site/style.css 3.84 kB 0 B
build/edit-widgets/index.js 21.1 kB 0 B
build/edit-widgets/style-rtl.css 3 kB 0 B
build/edit-widgets/style.css 3 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.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.49 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 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 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.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 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.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@StevenDufresne StevenDufresne marked this pull request as ready for review July 23, 2020 07:00
@StevenDufresne StevenDufresne requested a review from ellatrix as a code owner July 23, 2020 07:00
@dd32
Copy link
Member

dd32 commented Jul 24, 2020

@StevenDufresne Do you consider this a 5.5 blocker, or as I suspect, it can wait for 5.5.1?

@StevenDufresne
Copy link
Contributor Author

It's tough to tell but probably not a blocker seeing that it seems to be specific to blocks with parent/child relationships and at this moment in time there are few blocks written that way.

@ryelle
Copy link
Contributor

ryelle commented Aug 3, 2020

For some reason, this only works when I'm in the heading section of Cornell Notes. Once I move into the main section I can search the block directory again Nevermind this, the block can be aded here, so this is technically correct.

Screen Shot 2020-08-03 at 3 47 43 PM

I'm also able to search the block directory within this restricted container block, and like you mention above, it shows like this even though Waves is installed.

Screen Shot 2020-08-03 at 3 49 34 PM

@StevenDufresne StevenDufresne marked this pull request as draft August 3, 2020 23:21
@StevenDufresne
Copy link
Contributor Author

@ryelle Thanks for taking a look. I'll investigate some of the other pathways. In the meantime, I've switched this to a draft.

@StevenDufresne StevenDufresne marked this pull request as ready for review August 19, 2020 06:15
@StevenDufresne
Copy link
Contributor Author

@ryelle Can you test this again? Changes, I think made in #24429, don't allow the inserter to be open while I click inside.

@ryelle
Copy link
Contributor

ryelle commented Aug 19, 2020

Changes, I think made in #24429, don't allow the inserter to be open while I click inside.

I pulled this branch and locally rebased it on master, and I still see the same as above. Here's a screencast of the allowedBlocks issue, in case that's not clear: https://cloudup.com/cQU24Wu9_3t

On this branch I see the expected (no blocks found) when in the Cornell Notes header, though TBH I'm not sure what the difference is between my example block above and the Cornell Notes block that makes it work here but not there.

Screen Shot 2020-08-19 at 12 40 19 PM

@StevenDufresne
Copy link
Contributor Author

@ryelle Thanks for the screenshot. That was definitely helpful and I see what you are saying.

I don't think that behavior is necessarily connected to the problem this PR is trying to address. This PR makes sure we don't search the block directory when we are focused 'in' a block. Now, the fact that you can't have active focus on a parent block and the main inserter open, since #24429, this problem goes away from a user's perspective (the quick inserter doesn't support the block directory yet). The code is still a problem should we ever revert back the functionality from #24429.

Looking at the video, as you click to open the Inserter the focus is lost from the container and therefore isn't restricted and all blocks can return. That's correct.

The fact that the block you installed shows up in search is the problem you filed in #24560. It's related to the way the inserter filters out blocks.

@ryelle
Copy link
Contributor

ryelle commented Aug 19, 2020

Looking at the video, as you click to open the Inserter the focus is lost from the container and therefore isn't restricted and all blocks can return.

No, the currently selected block is still the restricted parent— you can see before I start typing in the video that only the 3 block types are available to me.

Now, the fact that you can't have active focus on a parent block and the main inserter open, since #24429, this problem goes away from a user's perspective

I'm not sure what you mean here. The cursor's focus moves out of the block, but the editor still tracks that block as "selected".

@StevenDufresne StevenDufresne force-pushed the fix/block-directory-inserter-result-persistence branch from 609eb9c to e869633 Compare August 20, 2020 00:22
@StevenDufresne
Copy link
Contributor Author

StevenDufresne commented Aug 20, 2020

Looking at the video, as you click to open the Inserter the focus is lost from the container and therefore isn't restricted and all blocks can return.

No, the currently selected block is still the restricted parent— you can see before I start typing in the video that only the 3 block types are available to me.

Now, the fact that you can't have active focus on a parent block and the main inserter open, since #24429, this problem goes away from a user's perspective

I'm not sure what you mean here. The cursor's focus moves out of the block, but the editor still tracks that block as "selected".

You are correct. I missed that it's only showing the restricted blocks. The difference is most likely that you don't have a parent/child relationship. This PR stops the searches when there are children blocks. I'll look into stopping if it has allowedBlocks as well.

@StevenDufresne StevenDufresne force-pushed the fix/block-directory-inserter-result-persistence branch 2 times, most recently from 11b8ff3 to 1c679d5 Compare September 14, 2020 04:12
@StevenDufresne StevenDufresne force-pushed the fix/block-directory-inserter-result-persistence branch from 1c679d5 to 8244866 Compare October 5, 2020 00:27
@StevenDufresne
Copy link
Contributor Author

@ryelle Do you mind taking a look at this again when you get a chance?

@ryelle
Copy link
Contributor

ryelle commented Oct 7, 2020

I don't think we can assume having allowedBlocks will mean that block-directory blocks would not be allowed, so just disabling the BD results doesn't really fix the larger problem. We should be checking "permission" for each block before showing them to the user. I think I've managed to do that in this PR: #25926

@StevenDufresne
Copy link
Contributor Author

@ryelle Yep, I agree. That branch is a better solution. Closing in favor of #25926.

@aristath aristath deleted the fix/block-directory-inserter-result-persistence branch November 10, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants