Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions src/wp-includes/icons.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php
/**
* Icons API: Icon rendering helper functions.
*
* @package WordPress
* @subpackage Icons
* @since 7.1.0
*/

/**
* Returns the SVG markup for a registered icon.
*
* @since 7.1.0
*
* @param string $name The namespaced icon name (e.g. 'core/plus',
* 'core/arrow-down', 'my-plugin/custom-icon').
* @param array $args {
* Optional. Arguments for the icon. Default empty array.
*
* @type int|null $size Width and height in pixels. Pass null to leave the
* SVG's intrinsic dimensions untouched. Default 24.
* @type string $class Additional CSS class names. Multiple classes may be
* provided as a space-separated string. Default empty string.
* @type string $label Accessible label. If provided, the SVG gets
* role="img" and aria-label. If omitted, the SVG
* gets aria-hidden="true" and focusable="false".
* Default empty string.
* }
* @return string SVG markup for the icon, or empty string if not found.
*/
function wp_get_icon( $name, $args = array() ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but we may want to refactor render_block_core_icon() to use this function instead of reinventing some of the same logic.

$icon = WP_Icons_Registry::get_instance()->get_registered_icon( $name );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to check if $name is not empty. WDYT?

if ( empty( $name ) ) {
	return;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since get_registered_icon() returns null if $name is empty, adding an empty check feels redundant.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we know about the registered icons? If this is a public API where arbitrary SVG content (that passes KSES checks) may be in the icon, things can get very complicated. One that comes to mind is if the SVG has a viewBox, then a single size argument may be tricky. Are icons required to be square? Is that enforced?

And just to confirm, anything in the registry has passed through KSES, right?

if ( is_null( $icon ) ) {
return '';
}

$svg = $icon['content'];
if ( empty( $svg ) ) {
return '';
}

$args = wp_parse_args(
$args,
array(
'size' => 24,
'class' => '',
'label' => '',
)
);

$processor = new WP_HTML_Tag_Processor( $svg );
if ( ! $processor->next_tag( 'svg' ) ) {
return '';
}

if ( is_numeric( $args['size'] ) ) {
$size = absint( $args['size'] );
$processor->set_attribute( 'width', (string) $size );
$processor->set_attribute( 'height', (string) $size );
}

if ( ! empty( $args['class'] ) ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine if we pass multiple classes there (class-1 class-2)? To be precise and use the API correctly, should we explode() those and call add_class() for each of them? If you agree, let's also add a test for that.

foreach ( preg_split( '/\s+/', $args['class'], -1, PREG_SPLIT_NO_EMPTY ) as $class_name ) {
$processor->add_class( $class_name );
}
}
Comment on lines +62 to +66

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a single class, or could it be multiple classes? The ::add_class method expects a single class, and multiple classes should be applied individually.

Something like $processor->add_class( 'wp-icon class2 class3' ) is not how the method is intended to be used.


if ( ! empty( $args['label'] ) ) {
$processor->set_attribute( 'role', 'img' );
$processor->set_attribute( 'aria-label', $args['label'] );
$processor->remove_attribute( 'aria-hidden' );
$processor->remove_attribute( 'focusable' );
} else {
$processor->set_attribute( 'aria-hidden', 'true' );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For decorative icons, should we also disable focusable to match what render_block_core_icon() is doing?

Suggested change
$processor->set_attribute( 'aria-hidden', 'true' );
$processor->set_attribute( 'aria-hidden', 'true' );
$processor->set_attribute( 'focusable', 'false' );

$processor->set_attribute( 'focusable', 'false' );
$processor->remove_attribute( 'role' );
$processor->remove_attribute( 'aria-label' );
}
Comment on lines +68 to +78

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider that the svg content may already have these attributes, so if one branch should have role and aria-label but not aria-hidden, and vice-versa for the other branch, then it's a good idea to remove the attributes from the other branch so we don't wind up with:

<svg aria-hidden="true" role="img" aria-label="whoops?" />
Suggested change
if ( ! empty( $args['label'] ) ) {
$processor->set_attribute( 'role', 'img' );
$processor->set_attribute( 'aria-label', $args['label'] );
} else {
$processor->set_attribute( 'aria-hidden', 'true' );
}
if ( ! empty( $args['label'] ) ) {
$processor->set_attribute( 'role', 'img' );
$processor->set_attribute( 'aria-label', $args['label'] );
$processor->remove_attribute( 'aria-hidden' )
} else {
$processor->set_attribute( 'aria-hidden', 'true' );
$processor->remove_attribute( 'role' );
$processor->remove_attribute( 'aria-label' )
}

(Do check me and test this, consider the suggestion pseudo-code)


return $processor->get_updated_html();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, are we intentional about not adding a filter here?

}
1 change: 1 addition & 0 deletions src/wp-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@
require ABSPATH . WPINC . '/class-wp-connector-registry.php';
require ABSPATH . WPINC . '/connectors.php';
require ABSPATH . WPINC . '/class-wp-icons-registry.php';
require ABSPATH . WPINC . '/icons.php';
require ABSPATH . WPINC . '/widgets.php';
require ABSPATH . WPINC . '/class-wp-widget.php';
require ABSPATH . WPINC . '/class-wp-widget-factory.php';
Expand Down
125 changes: 125 additions & 0 deletions tests/phpunit/tests/icons/wpGetIcon.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
<?php
/**
* Unit tests covering the wp_get_icon() function.
*
* @package WordPress
* @subpackage Icons
* @since 7.1.0
*
* @group icons
*
* @covers ::wp_get_icon
*/
class Tests_Icons_WpGetIcon extends WP_UnitTestCase {

/**
* @ticket 64847
*/
public function test_wp_get_icon_returns_svg_for_known_icon() {
$output = wp_get_icon( 'core/plus' );
$this->assertStringStartsWith( '<svg ', $output );
$this->assertStringContainsString( '</svg>', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_returns_empty_string_for_unknown_icon() {
$output = wp_get_icon( 'this-icon-does-not-exist' );
$this->assertSame( '', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_default_attributes() {
$output = wp_get_icon( 'core/plus' );
// WP_HTML_Tag_Processor lowercases attribute names.
$this->assertStringContainsString( 'viewbox="0 0 24 24"', $output );
$this->assertStringContainsString( 'width="24"', $output );
$this->assertStringContainsString( 'height="24"', $output );
$this->assertStringContainsString( 'aria-hidden="true"', $output );
$this->assertStringContainsString( 'focusable="false"', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_custom_size() {
$output = wp_get_icon( 'core/plus', array( 'size' => 32 ) );
$this->assertStringContainsString( 'width="32"', $output );
$this->assertStringContainsString( 'height="32"', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_size_null_leaves_dimensions_untouched() {
$output = wp_get_icon( 'core/plus', array( 'size' => null ) );
$this->assertStringNotContainsString( 'width=', $output );
$this->assertStringNotContainsString( 'height=', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_size_zero_outputs_zero_dimensions() {
$output = wp_get_icon( 'core/plus', array( 'size' => 0 ) );
$this->assertStringContainsString( 'width="0"', $output );
$this->assertStringContainsString( 'height="0"', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_custom_class() {
$output = wp_get_icon( 'core/plus', array( 'class' => 'my-button-icon' ) );
$this->assertStringContainsString( 'class="my-button-icon"', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_multiple_classes() {
$output = wp_get_icon( 'core/plus', array( 'class' => 'foo bar baz' ) );
$this->assertStringContainsString( 'class="foo bar baz"', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_with_label() {
$output = wp_get_icon( 'core/plus', array( 'label' => 'Add item' ) );
$this->assertStringContainsString( 'role="img"', $output );
$this->assertStringContainsString( 'aria-label="Add item"', $output );
$this->assertStringNotContainsString( 'aria-hidden', $output );
$this->assertStringNotContainsString( 'focusable', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_without_label_is_hidden() {
$output = wp_get_icon( 'core/plus' );
$this->assertStringContainsString( 'aria-hidden="true"', $output );
$this->assertStringContainsString( 'focusable="false"', $output );
$this->assertStringNotContainsString( 'role="img"', $output );
$this->assertStringNotContainsString( 'aria-label', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_contains_svg_content() {
$output = wp_get_icon( 'core/plus' );
$this->assertStringContainsString( '<path ', $output );
}

/**
* @ticket 64847
*/
public function test_wp_get_icon_escapes_attributes() {
$output = wp_get_icon( 'core/plus', array( 'class' => '"><script>alert(1)</script>' ) );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test escaping in label and aria-label and any other user-controller attribute?

$this->assertStringNotContainsString( '<script>', $output );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected form here? &lt;script&gt;? Might make sense to add some more assertions or just change to a positive assertion that matches what exactly we expect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably add some more assertion like assertEqualHTML for things like assertHTMLContainsNoTags( [ 'script' ], $html ) or similar.

For this case, it may be a good idea to create a tag processor with something like this, what we care about is not having a SCRIPT tag:

$p = new WP_HTML_Tag_Processor('<svg><script></script>');
while ( $p->next_token() ) {
  var_dump( $p->get_token_name() );
  if ( 'SCRIPT' === $p->get_token_name() ) {
    echo 'TEST FAILURE';
    throw new Error('bad bad bad');
  }
}
// assert $p->paused_at_incomplete_token() === false;

Or it may be better to just assert that that class was set:

$p = new WP_HTML_Tag_Processor('<hax0r>');
$p->next_tag();
$p->add_class('"><script>alert(1)</script>');
$malicious = $p->get_updated_html();

$p = new WP_HTML_Tag_Processor($malicious);
$p->next_tag();
assert( $p->has_class( '"><script>alert(1)</script>' ) );
echo 'test passed';

}
}
Loading