Twenty Seventeen: Fixes in SVG markup

[sami.keijonen]

I'm really sorry but I forgot that in IE SVG icons don't show up in the Customizer. This is two folded (is that a word) issue.

1. In inc/icon-functions.php we use absolute path in the Customizer so that icons show up in there.
2. But in IE absolute path don't work without JS polyfill.

But this issue is resolved in ticket #30028 which means that we can remove absolute path. Also ticket #35824 is related.

Other markup changes

Even if Twenty17 doesn't use title and desc anywhere I want this theme to show example how you could do it in accessible way. I'm proposing this:

	if ( $args['title'] && $args['desc'] ) {
		$unique_id       = uniqid();
		$aria_labelledby = ' aria-labelledby="title-' . $unique_id . ' desc-' . $unique_id . '"';
		$aria_hidden     = '';
	}

	// Begin SVG markup.
	$svg = '<svg class="icon icon-' . esc_attr( $args['icon'] ) . '"' . $aria_hidden . $aria_labelledby . ' role="img">';

	// If there is title and description, display them.
	if ( $args['title'] && $args['desc'] ) {
		$svg .= '<title id="title-' . $unique_id . '">' . esc_html( $args['title'] ) . '</title>';
		$svg .= '<desc id="desc-' . $unique_id . '">' . esc_html( $args['desc'] ) . '</desc>';
	}

I'm not sure do we need both, title and desc. Could also be or.

I'll send patch so that you can what I'm suggesting. It also fixes the issue in ticket #38387.

[sami.keijonen]

Now there is patch added. Summary of changes:

• Remove aria-hidden argument. Let there be aria-hidden="true"' by default and set it empty when there is title and desc around.
• Add unique IDs for title and desc for a11y example. Not sure if it's the best way.
• Remove absolute path in the Customizer. It doesn't work in IE and the original bug is fixed in #30028.
• Note that other solution is always use absolute path and ​svgxusesvgxuse.js for IE. From my undestanding this would help cache the SVG file.
• Use whitespace around <use>. See #38387. Props @swissspidy.

[afercia]

I'd consider to make the description optional. The function should allow to use just the title so something like:

if title
remove aria hidden, set aria-labelledby to title, and print the title

if description
check if a title is set, if not set don't do anything (a description should be printed out just if there's a title IMHO)
it a title is set, remove aria hidden set aria-labelledby to title and desc, and print both

Quickly tested on Safari 10+VoiceOver, the title and desc are announced.
Unfortunately Safari 10 seems to don't recognise the role img and thus VoiceOver always announces the svg as "group" (I've tried several things, with no luck). Not sure there's much we can do here.

[sami.keijonen]

In 38659.1.patch there is a little modified SVG markup as we talked. I kind of run out of time but can test more tomorrow.

Tested quickly with these.

<?php echo twentyseventeen_get_svg( array( 'icon' => 'arrow-right', 'title' => __( 'This is title', 'twentyseventeen' ) ) ); // Title gets output. ?>

<?php echo twentyseventeen_get_svg( array( 'icon' => 'arrow-right', 'desc' => __( 'This is desc', 'twentyseventeen' ) ) ); // Nothing gets output. ?>
			
<?php echo twentyseventeen_get_svg( array( 'icon' => 'arrow-right', 'title' => __( 'This is title', 'twentyseventeen' ), 'desc' => __( 'This is longer desc', 'twentyseventeen' ) ) ); // Title and desc gets output. ?>

[davidakennedy]

I tested this in a variety of browsers, and it looks good to me.

Noting that it contains the patch for #38387. So I need to make sure props are correct if committing this one.

Also, #38387 can become a ticket on whether to change xlink or not.

[laurelfulford]

Looks good to me, too!

Made some minor text updates to the comment description that goes with the examples in 38659.2.patch​.

[sami.keijonen]

Thanks @laurelfulford for better comments.

[davidakennedy]

In 39164:

Twenty Seventeen: Improve SVG markup, providing more options customization

• Removes aria-hidden argument. Lets aria-hidden="true" be there by default and sets it empty when there is title and desc.
• Adds unique IDs for title and desc for accessible implementation options.
• Removes absolute path in the Customizer. It didn't work in Internet Explorer, and the original bug is fixed in #30028.
• Add whitespace around <use>, from #38387.

Props sami.keijonen, swissspidy, laurelfulford.

Fixes #38659.
See #38387.