Opened 5 years ago
Closed 5 months ago
#4730 closed defect (bug) (fixed)
unbalanced HTML in Plugins Home page
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | Priority: | normal | |
| Component: | Plugin Directory | Keywords: | good-first-bug has-patch |
| Cc: |
Description
If you run https://wordpress.org/plugins/ through a validator, you get quite a mess.
The main things are:
- stray
</span>tag for each plugin shown - duplicate ID
post-xxxxxandplugin-slugfor each (?) <style>not allowed as child of element<a>for each plugin- unclosed
<div>(for div with id="page")
Both https://validator.w3.org/nu/ and https://html5.validator.nu/ gave those errors, in addition to lots of other warnings.
Attachments (1)
Change History (28)
#2
@
5 years ago
AFAIK we can't use srcset as most of these need to be explicitly background images, which is why it's CSS in the first place.
We can probably drop the ID's though and switch to classes, so it won't matter if a plugin appears multiple times or has multiple <style>'s output.
This ticket was mentioned in Slack in #meta by joyously. View the logs.
5 years ago
This ticket was mentioned in Slack in #meta by joyously. View the logs.
5 years ago
#11
@
5 years ago
- Resolution set to fixed
- Status changed from new to closed
Fixed the issues that are fixable.
<style> not allowed as child of element <a> for each plugin is going to stay, because the only way to "fix" it is to remove the <style> from the body entirely as it's "improper" to be there and should only ever be in the <head> - But realistically, browsers don't care. Apparently the ability to use <style> within <body> was removed from the HTML5 draft. :shrug:
#12
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Actually, you're right, apologies @joyously - We probably can switch these plugin icons to an <img> element with srcset..
It's the not-proper-sized images which we have trouble with, and the banner header on singular pages. The banner image is a background image for reasons I can't recall now, but using an <img> element there wasn't going to work, I'm not sure what it was.
We're not going to remove the inline <style> tags everywhere, but slimming this particular piece of HTML down might be worth it.
#16
follow-up:
↓ 18
@
5 years ago
Thanks @dufresnesteven.
I'm actually not even sure that this should have an alt, and if it should, if it shouldn't just be an alt="".
The image is almost purely for decoration, and the plugin name will always be included next to/after it in terms of a screen reader, so I'm actually thinking that this will increase the burden upon the viewer without actually providing any benefit of context. Thoughts?
The %s plugin logo
As an FYI, I think this should only be referred to as the plugin icon since it's not always the logo of the plugin, and may also be auto-generated.
#17
@
5 years ago
Actually, it is recommended to use alt="" for decorative images. Here's one reference: https://www.w3.org/WAI/tutorials/images/decorative/
#18
in reply to:
↑ 16
;
follow-up:
↓ 20
@
5 years ago
There is definitely debate as to whether an icon that does have text next to it needs an alt tag and based on the component itself, I would agree it may not be necessary. But if we look at this component in the flow, it isn't apparent.
For example, on the /plugins page, tabbing through doesn't go through the expected flow. Instead of reading the Heading to contextualize the section, we go directly to the "See All" link and then to the plugin itself.
It sounds like this when you tab into the Block Enabled Plugins section:
"Main section heading: See all block enabled plugins, link"
Tab to select a plugin
"Article"
Tab to icon
"Icon XXXXXX, link"
Tab to title
It then reads the name of the plugin.
If the plugin doesn't have the word "Plugin" in it, theres no way to tell you are focused on a plugin.
You can try this extension to validate:
https://chrome.google.com/webstore/detail/chromevox-classic-extensi/kgejglhpjiefppelpmljglcjbhoiplfn
Note: we should probably also add a title to the <a> tag that surrounds the <img> since there is no way to tell where it's going.
Replying to dd32:
Thanks @dufresnesteven.
I'm actually not even sure that this should have an alt, and if it should, if it shouldn't just be an
alt="".
The image is almost purely for decoration, and the plugin name will always be included next to/after it in terms of a screen reader, so I'm actually thinking that this will increase the burden upon the viewer without actually providing any benefit of context. Thoughts?
The %s plugin logo
As an FYI, I think this should only be referred to as the plugin icon since it's not always the logo of the plugin, and may also be auto-generated.
#19
@
5 years ago
Actually I am wrong about adding a title to the anchor link :)
https://silktide.com/blog/2013/i-thought-title-text-improved-accessibility-i-was-wrong
#20
in reply to:
↑ 18
;
follow-up:
↓ 21
@
5 years ago
Replying to dufresnesteven:
It sounds like this when you tab into the Block Enabled Plugins section:
"Main section heading: See all block enabled plugins, link"
Tab to select a plugin
"Article"
Tab to icon
"Icon XXXXXX, link"
Tab to title
It then reads the name of the plugin.
To me, that sounds like we either need to hide the <a> for the icon from screen readers (There's probably an aria-* tag for that?) or move the <img> into the plugin links <a>?
#21
in reply to:
↑ 20
@
5 years ago
Yep, I think that would definitely be the best approach for the plugin items but it still doesn't address that fact that the pages that surround these plugin items don't have proper tabbing and context. Unless we are willing to go back and address the pages that surround these items, I believe more information is really the best solution. With that being said, I have plenty to learn when it comes to accessibility, do we have any experts around?
Replying to dd32:
To me, that sounds like we either need to hide the
<a>for the icon from screen readers (There's probably anaria-*tag for that?) or move the<img>into the plugin links<a>?
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
19 months ago
#23
@
19 months ago
The image tags require an alt attribute. Leaving it empty as alt='' in the get_plugin_icon() function is an option, though. Then when an image is linked, either the link would need to include nearby visible text or else meaningful alternative text would replace the empty alt=''.
In addition to the Plugin Directory page, this also affects embeds for plugins and themes. @alh0319 mentioned the embeds in Slack, in relation to the content on https://gdwp20.com/. Should those be handled on a separate ticket (or two, dividing plugin and theme embeds)? My draft PR proposes several changes together (it may still need the phpcs:ignore and/or further editing).
This ticket was mentioned in PR #136 on WordPress/wordpress.org by @sabernhardt.
19 months ago
#24
- Keywords has-patch added; needs-patch removed
For #4730
- Adds empty
altattributes to theget_plugin_icon()function. - Uses the plugin name as alt text on Plugins Directory page.
- Moves the plugin icon inside the link with visible text for embeds, and adjusts CSS.
- Hides dashicons from screen readers, both on Plugins Directory page and in plugin embeds.
- Moves theme's thumbnail image inside the link with visible text for embeds, and adds an empty
altattribute.
</span>tag for each plugin is in the themehttps://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/themes/pub/wporg-plugins/template-parts/plugin.php#L60
post-xxxxxandplugin-slugI think this is because some plugins can show in multiple sections since they are
order_by rand.<style>not allowed as child of element<a>The style is output here:
https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/themes/pub/wporg-plugins/client/components/plugin-icon/index.jsx#L37
Perhaps it should be
srcsetinstead?<div>(for div with id="page") is in the themeIt should be here:
https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/themes/pub/wporg-plugins/footer.php#L17