Interactivity API: fix property modification from inherited context two or more levels above #66872

DAreRodz · 2024-11-08T13:43:16Z

What?

The Interactivity API context can be nested, and inherited properties are supposed to be modified in the contexts where they are defined. This was not the case for properties inherited from contexts defined two or more levels above.

More info at #65623 (comment).

Fixes #65623.

This PR also fixes a bug in the Navigation block that seems related to the bug fix; see #66872 (comment).

How?

Applying the proposed fix mentioned in #65623 (comment).

Testing Instructions

You can follow the "Step-by-step reproduction instructions" at #65623 (comment) to reproduce the issue and check it disappears with the bug fix.

I've added unit tests to ensure the issue has been addressed and prevent it from happening again.

github-actions · 2024-11-08T13:45:19Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: danielpost <danielpost@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

sirreal · 2024-11-08T15:59:59Z

packages/interactivity/src/proxies/context.ts

+	has: ( target, key ) =>
+		Reflect.has( target, key ) ||
+		Reflect.has( contextObjectToFallback.get( target ), key ),


gutenberg/packages/interactivity/src/proxies/context.ts

Line 27 in 636710b

const obj = key in target || ! ( key in fallback ) ? target : fallback;

And the problem arises when the key in fallback expression is evaluated.

I think the solution is to define a trap for the [[HasProperty]] internal method, returning true for inherited properties. We already have a trap for [[OwnPropertyKeys]] that returns the list of "own keys," which includes the inherited properties as well.
(from #65623 (comment))

This is confusing, but I think it makes sense. It does fix the issue in my testing. Are there tests already for the case where the value should be set on fallback?

With all the overrides to object behavior with proxies, it probably makes sense to add more special behaviors.

I was confused a bit since the has proxy changes the behavior of the in operator. It appears that it would change the behavior of the lines above, however those lines are on the unproxied objects so that usage of in is not impacted. Is that correct?

those lines are on the unproxied objects so that usage of in is not impacted. Is that correct?

Correct. 🙂

Are there tests already for the case where the value should be set on fallback?

Yes, there are. We have a couple of unit tests checking prop modifications on fallback contexts:

gutenberg/packages/interactivity/src/proxies/test/context-proxy.ts

Lines 77 to 158 in 67b7e04

describe( 'set', () => {

it( 'should modify props defined in it', () => {

const fallback: any = proxifyContext(

{ prop: 'fallback' },

{}

);

const context: any = proxifyContext(

{ prop: 'context' },

fallback

);

context.prop = 'modified';

expect( context.prop ).toBe( 'modified' );

expect( fallback.prop ).toBe( 'fallback' );

} );

it( 'should modify props inherited from its fallback', () => {

const fallback: any = proxifyContext(

{ prop: 'fallback' },

{}

);

const context: any = proxifyContext( {}, fallback );

context.prop = 'modified';

expect( context.prop ).toBe( 'modified' );

expect( fallback.prop ).toBe( 'modified' );

} );

it( 'should see changes in inherited props', () => {

const fallback: any = proxifyContext(

{ prop: 'fallback' },

{}

);

const context: any = proxifyContext( {}, fallback );

fallback.prop = 'modified';

expect( context.prop ).toBe( 'modified' );

expect( fallback.prop ).toBe( 'modified' );

} );

it( 'should create non-inherited props in itself', () => {

const fallback: any = proxifyContext( {}, {} );

const context: any = proxifyContext( {}, fallback );

context.prop = 'modified';

expect( context.prop ).toBe( 'modified' );

expect( fallback.prop ).toBeUndefined();

} );

it( 'should work with the proxified state', () => {

const state = proxifyState( 'test', { a: 1 } );

const fallback: any = proxifyContext( state, {} );

const context: any = proxifyContext( {}, fallback );

context.a = 2;

expect( context.a ).toBe( 2 );

expect( state.a ).toBe( 2 );

} );

it( "should modify props inherited from fallback's ancestors", () => {

const ancestor: any = proxifyContext(

{ ancestorProp: 'ancestor' },

{}

);

const fallback: any = proxifyContext(

{ fallbackProp: 'fallback' },

ancestor

);

const context: any = proxifyContext( {}, fallback );

context.ancestorProp = 'modified';

expect( context.ancestorProp ).toBe( 'modified' );

expect( fallback.ancestorProp ).toBe( 'modified' );

expect( ancestor.ancestorProp ).toBe( 'modified' );

} );

} );

sirreal · 2024-11-08T16:12:02Z

Reset to draft, @DAreRodz is investigating the navigation block issue: #65623 (comment)

DAreRodz · 2024-11-12T10:40:12Z

Okay, this is interesting. The failing test passes without the fix, but the tested behavior (tab outside a nested submenu) doesn't work when the case is manually reproduced, both with and without the fix. 🤔

DAreRodz · 2024-11-12T13:51:44Z

I've found it! It was the modal property inside submenu contexts. The property should be defined explicitly to prevent it from being inherited and thus manipulated in a parent context.

Note that although tests passed before without this bug fix, it seemed the tested behavior I mentioned above (using the tab key to move outside a nested submenu) was broken anyway.

In `trunk` (with tests passing)	After `78c8b20`
Screen.Recording.2024-11-12.at.14.36.31.mov	Screen.Recording.2024-11-12.at.14.38.14.mov

…wo or more levels above (WordPress#66872) * Add failing test * Fix code * Add changelog entry * Define "modal" prop in submenu context Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: danielpost <danielpost@git.wordpress.org>

…wo or more levels above (#66872) * Add failing test * Fix code * Add changelog entry * Define "modal" prop in submenu context Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: danielpost <danielpost@git.wordpress.org>

* Interactivity API: fix property modification from inherited context two or more levels above (#66872) * Add failing test * Fix code * Add changelog entry * Define "modal" prop in submenu context Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: danielpost <danielpost@git.wordpress.org> * Remove last ts refactor --------- Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: danielpost <danielpost@git.wordpress.org>

t-hamano · 2025-04-23T08:33:22Z

Removing Backport to WP Minor Release label because #67259, which is equal to this PR, has already been merged into wp/6.7 branch.

DAreRodz added 2 commits November 8, 2024 14:23

Add failing test

67b7e04

Fix code

9065d12

DAreRodz added [Type] Bug An existing feature does not function as intended [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Nov 8, 2024

DAreRodz self-assigned this Nov 8, 2024

Add changelog entry

087110f

DAreRodz requested review from sirreal and michalczaplinski November 8, 2024 13:45

DAreRodz marked this pull request as ready for review November 8, 2024 13:45

DAreRodz requested a review from luisherranz as a code owner November 8, 2024 13:45

DAreRodz mentioned this pull request Nov 8, 2024

Interactivity API: Context not updating properly from radio button click with nested contexts #65623

Closed

2 tasks

DAreRodz marked this pull request as draft November 8, 2024 14:27

sirreal approved these changes Nov 8, 2024

View reviewed changes

sirreal marked this pull request as ready for review November 8, 2024 16:10

sirreal marked this pull request as draft November 8, 2024 16:11

sirreal self-requested a review November 8, 2024 16:12

Define "modal" prop in submenu context

78c8b20

DAreRodz marked this pull request as ready for review November 12, 2024 13:52

DAreRodz requested a review from ajitbohra as a code owner November 12, 2024 13:52

DAreRodz added the [Block] Submenu Affects the Submenu Block - for submenus in navigation label Nov 12, 2024

DAreRodz merged commit d2bc9ea into trunk Nov 12, 2024
69 checks passed

DAreRodz deleted the fix/iapi-context-inheritance-from-fallback-ancestor branch November 12, 2024 14:12

github-actions bot added this to the Gutenberg 19.7 milestone Nov 12, 2024

cbravobernal added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 12, 2024

cbravobernal mentioned this pull request Nov 24, 2024

Interactivity API: fix property modification backport to 6.7.2 #67259

Merged

luisherranz mentioned this pull request Feb 19, 2025

Interactivity API iteration for WordPress 6.8 #67276

Closed

49 tasks

t-hamano mentioned this pull request Apr 5, 2025

Navigation block's "Open on click" does not work for nested submenus #60738

Closed

t-hamano removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Apr 23, 2025

t-hamano mentioned this pull request May 29, 2025

Incorrect keyboard operation of navigation blocks with nested submenus. #64626

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Interactivity API: fix property modification from inherited context two or more levels above #66872

Interactivity API: fix property modification from inherited context two or more levels above #66872

Uh oh!

DAreRodz commented Nov 8, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 8, 2024 •

edited

Loading

Uh oh!

sirreal Nov 8, 2024

Uh oh!

DAreRodz Nov 12, 2024

Uh oh!

sirreal commented Nov 8, 2024

Uh oh!

DAreRodz commented Nov 12, 2024

Uh oh!

DAreRodz commented Nov 12, 2024 •

edited

Loading

Uh oh!

Uh oh!

t-hamano commented Apr 23, 2025

Uh oh!

Uh oh!

	describe( 'set', () => {
	it( 'should modify props defined in it', () => {
	const fallback: any = proxifyContext(
	{ prop: 'fallback' },
	{}
	);
	const context: any = proxifyContext(
	{ prop: 'context' },
	fallback
	);

	context.prop = 'modified';

	expect( context.prop ).toBe( 'modified' );
	expect( fallback.prop ).toBe( 'fallback' );
	} );

	it( 'should modify props inherited from its fallback', () => {
	const fallback: any = proxifyContext(
	{ prop: 'fallback' },
	{}
	);
	const context: any = proxifyContext( {}, fallback );

	context.prop = 'modified';

	expect( context.prop ).toBe( 'modified' );
	expect( fallback.prop ).toBe( 'modified' );
	} );

	it( 'should see changes in inherited props', () => {
	const fallback: any = proxifyContext(
	{ prop: 'fallback' },
	{}
	);
	const context: any = proxifyContext( {}, fallback );

	fallback.prop = 'modified';

	expect( context.prop ).toBe( 'modified' );
	expect( fallback.prop ).toBe( 'modified' );
	} );

	it( 'should create non-inherited props in itself', () => {
	const fallback: any = proxifyContext( {}, {} );
	const context: any = proxifyContext( {}, fallback );

	context.prop = 'modified';

	expect( context.prop ).toBe( 'modified' );
	expect( fallback.prop ).toBeUndefined();
	} );

	it( 'should work with the proxified state', () => {
	const state = proxifyState( 'test', { a: 1 } );
	const fallback: any = proxifyContext( state, {} );
	const context: any = proxifyContext( {}, fallback );

	context.a = 2;

	expect( context.a ).toBe( 2 );
	expect( state.a ).toBe( 2 );
	} );

	it( "should modify props inherited from fallback's ancestors", () => {
	const ancestor: any = proxifyContext(
	{ ancestorProp: 'ancestor' },
	{}
	);
	const fallback: any = proxifyContext(
	{ fallbackProp: 'fallback' },
	ancestor
	);
	const context: any = proxifyContext( {}, fallback );

	context.ancestorProp = 'modified';

	expect( context.ancestorProp ).toBe( 'modified' );
	expect( fallback.ancestorProp ).toBe( 'modified' );
	expect( ancestor.ancestorProp ).toBe( 'modified' );
	} );
	} );

Interactivity API: fix property modification from inherited context two or more levels above #66872

Interactivity API: fix property modification from inherited context two or more levels above #66872

Uh oh!

Conversation

DAreRodz commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

How?

Testing Instructions

Uh oh!

github-actions bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sirreal Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

DAreRodz Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

sirreal commented Nov 8, 2024

Uh oh!

DAreRodz commented Nov 12, 2024

Uh oh!

DAreRodz commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

t-hamano commented Apr 23, 2025

Uh oh!

Uh oh!

DAreRodz commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

DAreRodz commented Nov 12, 2024 •

edited

Loading