Navigation Block: Add missing menu item attributes to core/navigation

[mikachan]

Description

The navigation-link block handles the following attributes if they are passed through to the block: url, opensInNewTab, rel, and title.

However, if the navigation block is used on its own for a menu, without any navigation-link block children (such as automatically populating a menu with an __unstableLocation set), the opensInNewTab, rel and title attributes are not passed through to the front end.

It looks like this is because these attributes are not handled in gutenberg_parse_blocks_from_menu_items, so this PR adds these 3 attributes to this function.

How has this been tested?

Add a navigation block on its own with an __unstableLocation:

<!-- wp:navigation {"itemsJustification":"right","isResponsive":true,"__unstableLocation":"primary"} /-->

You'll also need a menu set to the primary location. Then set at least one link to open in a new tab, set a title and set a link relationship:

Before:

After:

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• 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 (please manually search all *.native.js files for terms that need renaming or removal).

Fixes Automattic/themes#4813

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mikachan! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[talldan]

Not 100% sure, but I think it should be $menu_item->attr_title for the block's title attribute. Otherwise it ends up the same as the label.

There's a JavaScript version of this function that I referenced, and it probably indicates some other things are missing too:

gutenberg/packages/block-library/src/navigation/menu-items-to-blocks.js

Lines 114 to 166 in 5185ffc

	function menuItemToBlockAttributes( {
	title: menuItemTitleField,
	xfn,
	classes,
	// eslint-disable-next-line camelcase
	attr_title,
	object,
	// eslint-disable-next-line camelcase
	object_id,
	description,
	url,
	type: menuItemTypeField,
	target,
	} ) {
	// For historical reasons, the `core/navigation-link` variation type is `tag`
	// whereas WP Core expects `post_tag` as the `object` type.
	// To avoid writing a block migration we perform a conversion here.
	// See also inverse equivalent in `blockAttributesToMenuItem`.
	if ( object && object === 'post_tag' ) {
	object = 'tag';
	}
	return {
	label: menuItemTitleField?.rendered || '',
	...( object?.length && {
	type: object,
	} ),
	kind: menuItemTypeField?.replace( '_', '-' ) || 'custom',
	url: url || '',
	...( xfn?.length &&
	xfn.join( ' ' ).trim() && {
	rel: xfn.join( ' ' ).trim(),
	} ),
	...( classes?.length &&
	classes.join( ' ' ).trim() && {
	className: classes.join( ' ' ).trim(),
	} ),
	...( attr_title?.length && {
	title: attr_title,
	} ),
	// eslint-disable-next-line camelcase
	...( object_id &&
	'custom' !== object && {
	id: object_id,
	} ),
	...( description?.length && {
	description,
	} ),
	...( target === '_blank' && {
	opensInNewTab: true,
	} ),
	};
	}

[mikachan]

Not 100% sure, but I think it should be $menu_item->attr_title for the block's title attribute. Otherwise it ends up the same as the label.

Thanks, this makes sense! I've changed it and it looks like it works well.

Thanks also for pointing me in the direction of that JS function. I'll run through this and see if I can match up the other missing attributes.

[talldan]

This stood out to me as the link block doesn't have a classes attribute.

I think it'd be good to try to stick to block attributes so that the render code in the block doesn't need changes.

It might be an option to use className instead here, which is added by the customClassName hook by default to all blocks that don't opt out:

gutenberg/packages/block-editor/src/hooks/custom-class-name.js

Lines 33 to 35 in 39b4c96

	className: {
	type: 'string',
	},

By doing that it should also be possible to avoid the changes to the navigation-link/index.php. The array to string conversion would have to be moved from there to here.

[talldan]

When I check back on this PR I realised this comment above was still 'pending' (had probably been so for about three days). Better late than never!

[mikachan]

When I check back on this PR I realised this comment above was still 'pending' (had probably been so for about three days). Better late than never!

Haha no worries! Thanks for the pointer with className, I'll take a look at this next.

[talldan]

@mikachan Just double-checking, should this have been removed?

I was thinking that the navigation-link/index.php would be able to remain unchanged from trunk.

[mikachan]

I was seeing some of the class names being duplicated, which is why I removed this. I can't see where they're being added originally though.

With $attributes['className']:

Without $attributes['className']:

faq-class (a custom test class I've added from Appearance > Menus), menu-item, menu-item-type-post_type, and menu-item-object-page are all duplicated when $attributes['className'] is used here. If I change the attribute name to something else (in both navigation-link/index.php and navigation/index.php), then the class names are only included once, so I'm assuming something else is picking up on className and adding them. If I var_dump $attributes['className'], it only includes unique classes, and I'm struggling to find where the others are added.

Ideally, I'd like to leave navigation-link/index.php unchanged and fix the duplication problem. If you have time, it would be great if you have any ideas around this!

[talldan]

Ah, yep, you're right, good catch. It seems like a bug in trunk that a custom class name is applied twice (happens when using the navigation block normally in the block editor too), so this would be a fix.

I can't see where they're being added originally though.

I think it must be via the get_block_wrapper_attributes function, which is defined in WordPress core:
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-block-supports.php#L179

Since the block supports both generated and custom class names, those values get merged in like this:
https://github.com/WordPress/wordpress-develop/blob/b45c85a405ab0b1968380a79f7a76dc9293adc0f/src/wp-includes/block-supports/custom-classname.php#L44-L56

In that case it looks good to me, will approve the PR

[mikachan]

Ahh brilliant, I understand, thanks so much for explaining. Nice, unexpected bug fix!

Thanks for your help with this! 🎉

[talldan]

Thanks for improving this, and also fixing the bug with class names. 🎉