It would be better to include a stable reference to an empty array to avoid unnecessary re-renders when there are no bindable attributes.

1659414

If I recall correctly, the check was in place to cover for the case when someone adds bindings in metadata, but the block doesn't support bindings.

Yeah, that makes sense. This is the only part where I had to change the logic.

useBlockProps doesn't use useSelect (nor does it read from any stores at all AFAICS); this makes sense to me, since it's a fairly low-level tool. I didn't want to introduce a dependency on a store, even if that means that the block could be shown with a purple border in the editor because it has bindings in the metadata even though the block doesn't support bindings.

TBH I'm not even sure is useBlockProps (and a modification of the block style, rather than e.g. adding a class name) is that right place for this. I looked around a bit, and other code that highlights reusable blocks (e.g. patterns or template parts) tends to do that quite a bit differently.

I'm willing to look into tightening the logic so it'll really only highlight the block if it does support bindings, but it looked like it needed further research how to best do this, and it seemed like a small enough edge case that I didn't want to block this PR by it.

I don't consider it a blocker. If you find a better place to inject this special styling, that would be the best solution for me. It can be handled seperately.

@talldan, all e2e tests pass, but I would still appreciate some confirmation that this change doesn't introduce regressions. I noticed it when performing a post-merge check on trunk.

@gziolo Do you mean the change from || to &&? That does look kinda wrong now 🤔

Yes, it wasn't immediately apparent.

The || makes more sense to me. || would delete anything that's either a non-bindable attribute or has pattern overrides, which is what the comment above the code says.

With this change you only delete the attribute if it has a pattern overrides binding.

I'll file a fix for this tomorrow.

Oh, looks like the second part of the criterion was already removed altogether: 2a9598c#diff-bff98e322c4269c50aa13e4b251beb9d53d2d0c3d4066406f735d2d49bed83a1L269-L270

I'm not sure the new version is quite correct TBH:

Maybe we did this accidentally, since the && was giving the wrong behavior? We might want to revisit this and bring back the previous logic.

diff --git a/packages/block-editor/src/hooks/block-bindings.js b/packages/block-editor/src/hooks/block-bindings.js
index 87ab4e3632..a8cc0d369f 100644
--- a/packages/block-editor/src/hooks/block-bindings.js
+++ b/packages/block-editor/src/hooks/block-bindings.js
@@ -436,11 +436,14 @@ export const BlockBindingsPanel = ( { name: blockName, metadata } ) => {
        if ( ! bindableAttributes || bindableAttributes.length === 0 ) {
                return null;
        }
-       // Filter bindings to only show bindable attributes.
+       // Filter bindings to only show bindable attributes and remove pattern overrides.
        const { bindings } = metadata || {};
        const filteredBindings = { ...bindings };
        Object.keys( filteredBindings ).forEach( ( key ) => {
-               if ( ! bindableAttributes.includes( key ) ) {
+               if (
+                       ! bindableAttributes.includes( key ) ||
+                       filteredBindings[ key ].source === 'core/pattern-overrides'
+               ) {
                        delete filteredBindings[ key ];
                }
        } );

I'll be AFK on Thu and Fri; @gziolo @cbravobernal Maybe y'all could look into this?

Edit again:

The pattern overrides should never appear. As it doesn't contain an editorUI attribute. And when a block contains a pattern override, the __default attribute won't appear on the UI, as is not a "supported" attribute.

That check is not needed. I found another bug if there are no sources. We show the panel without a readonly attributes panel, which is not ideal.

<!-- wp:image {"metadata":{"bindings":{"url":{"source":"core/undefined"},"alt":{"source":"core/undefined"},"title":{"source":"core/post-data", "args":{"key":"date"}}}}} -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

Drafted a PR: #72253

Looking at code vitals, this PR made the "hover" metric slower. My guess is it's because of this addition here. I think it's also responsible of a small impact on the "type" metrics (even though it's hard to see there)

I think the reason (a guess, not 100% certain) is that we prefer to avoid adding new useSelect calls as much as possible to these components that run for every block. Now, it's possible to solve this as we have a "private edit context" (or something like that) to solve this. Basically we group all the useSelect calls that we need for a block at a single place.

Can we try moving this there to see?

(Just saw the previous related discussion)

@cbravobernal, is that on your radar for 6.9?

I've added it to the "Bugfixes" section in #67520.

@youknowriad Can you help me figure out where to put this specifically? The selector checks what attributes are supported by block bindings, which is information passed by the server (via getSettings()). Potentially, any block can have such attributes, which is why we're running this code for all blocks...

Maybe in this useSelect

@cbravobernal has filed a PR to address this: #72351

Getting errors in the site editor

Uncaught TypeError: Cannot read properties of undefined (reading 'includes')

Fix: #72401
Revert: #72400