Added filters for menu icons and improved accessibility on svg elements (icons)

[Lovor01]

What?

Added two commits:

1. two filters for menu icons (menu and close)
2. improved accessibility on svg elements (icons)

Why?

This addresses #37930 issue.

How?

Two filters in PHP render functions of block are added, developer can add their own icons via these filters.

Testing Instructions

Use new filters block_core_navigation_menu_toggle_icon and block_core_navigation_menu_close_icon in e.g. functions.php of the theme to return desired svg or img elements.

Testing Instructions for Keyboard

Not relevant

Screenshots or screencast

Not relevant

[Lovor01]

I am not sure about focusable attribute on <svg> icons - they were previously introduced, so I left them, however, this is not standardized feature and works only on IE (which we do not support anymore) and Edge. I would consider to remove them.

[Lovor01]

After submitting PR, I came to idea that it may be good thing to provide $attributes variable to menu filters as well. User may have multiple instances of menu blocks on page and want to have different menu icons on different blocks?

[alexstine]

@Lovor01 Could you please look in to the failing PHP unit tests?

Thanks.

[Lovor01]

Please ignore React Native E2E Test fail, I changed only PHP. They failed because iOS screen snippets failing to create, I am not sure why, but it has nothing to do with my proposed changes.

[alexstine]

@Lovor01 Looks good to me. Let me see if I can track down one more review. 👍

[alexstine]

@getdave Can you review this or tag someone who can?

Thanks.

[carolinan]

Can you please provide a complete code example as part of the testing instructions?

[alexstine]

@carolinan It would be something like this.

add_filter( 'block_core_navigation_menu_toggle_icon', function() {
	return '<svg custom-attrs-here />';
} );

AND

add_filter( 'block_core_navigation_menu_close_icon', function() {
	return '<svg custom-attrs-here />';
} );

Does this help?

[carolinan]

I have tested the PR, do I understand correctly that all it does is change the icon that is used on the front?
I personally don't see the benefit compared to using render_block.

I also don't think that it solves the need that is described in the initial issue? Since the user can not select the custom icon in the UI, it actually limits their options.

[alexstine]

@carolinan

I have tested the PR, do I understand correctly that all it does is change the icon that is used on the front?

All it does is add filters that allow you to pass in a custom SVG.

I also don't think that it solves the need that is described in the initial issue? Since the user can not select the custom icon in the UI, it actually limits their options.

I would not say it limits options but it is not a UI solution for sure. This is an implementation suited for developers who wish to use custom code to override the outputted SVG. Those filters could be added in a mu-plugin, plugin, or child theme.

Looks like render_block hook returns the full content of the block vs. the icon so this looks fairly tricky to adjust just the SVGs compared to what the solution in this PR allows.

https://developer.wordpress.org/reference/hooks/render_block/

I think what really needs to be answered here is should this be a UI feature or should this be adjustable for developers? If the first is true, this PR should be closed. If the second is true, I say this is a good solution.

Thanks.

[Lovor01]

Let me jump into discussion with just a quick note. There were requests for possibility to change menu icon, which is quite legitimate, some suggestions in 37930 propose filters. This is a quick fix for developers and it does not stand in a way of addition of javascript solution to give user possibility to upload images from editor. Proposed solution with filters can co-exist with javascript UI solution.

[Lovor01]

And if you do use render_block hook to change icons, regular expressions must be used to change block content, which is not so elegant solution as filters and regular expressions are not as fast. Also, block code may change in the future, which may result in regular expressions replacements stop functioning. Filters are more future-proof solution.

[getdave]

Thank you for this PR.

Tagging @ajlende for his expertise with SVG.

I'll also add to my review queue.

[ajlende]

Tagging @ajlende for his expertise with SVG.

The main point of concern around SVGs is security. Adding a filter that replaces the entire SVG like this puts the burden of security onto plugin/theme developers which, I think, is fine.

This is a quick fix for developers and it does not stand in a way of addition of javascript solution to give user possibility to upload images from editor. Proposed solution with filters can co-exist with javascript UI solution.

Let's say that this PR is merged as-is, a plugin/theme has used the filter, and later a solution for the UI is added to give the user control over the icon.

How do you propose that the filter co-exists with the new UI? In most cases for things like this, I think the filter is usually on the list of items that the user can select from rather than the final rendered output.

[getdave]

I'm not sure we want to introduce these filters as they may not be needed. I should be able to provide a PoC that will allow developers to update these icons using existing tools. I'll try and spin that up soon.

[nathanrodrigues2111]

Hi, @getdave

Hope you are doing well.

I am currently working on the same where I required a way to add more icons for the navigation hamburger. So would love to see a filter or workaround for the same if possible. You mentioned that you will be working PoC that will allow developers to update these icons using existing tools. So if there is any update to that then please let me know. As I am highly interested in knowing the same,

Thanks