Introduce a Site Health module for users to self-service their site

[Clorith]

One of the mentioned ​9 Projects for 2019, merging the ​Health Check plugin was noted as one of them.

Not everything in the plugin is great candidates for core, but the Site Health and Debug Information are both valuable entries. (The debug information has previously been covered in #39165, but will be superseded by this implementation, as that was for a standalone page).

The attached patch introduces multiple classes, filters, and tools for developers and users alike.

The Site Health section is meant for self servicing a website, it will alert you to known common issues and best practices.

Developers may use the health_check_site_status_tests filter to add their own tests, or remove tests (for example a host handles updates, then us testing that auto-updates are disabled is pointless, as it would be a false positive). More information on tests and such would go into devnotes for the release.

The debug information is essentially a quick way to look up information about your site and the host it uses, and also allows developers to add their own sections through the debug_information filter. Many of the larger plugins implement their own type of these. That means users have to look for information in various places, and they may have different needs, this centralizes the information gathering, and also provides the user an easy way to copy that information for sharing with supporters for example.

Some tests and features that exist in the plugin are not quite ready for core inclusion, and have been dropped from this patch, but may be introduced in future iterations with later versions.

[Clorith]

Site health status with failures and recommendations

[Clorith]

Site info/debug screen, with the copy field shown

[birgire]

Awesome, that looks great and has been polished a lot since #39165

Few things I noticed from a quick skimming of 46573.patch​ and first time testing the patch:

• The patch has many @uses tags inline documentation. The handbook says: This tag has been used in the past, but should no longer be used.: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#phpdoc-tags

• After having to mouse click many times, to open up the closed accordian sections, I started to wish for an option to open/close all at once.

• On my Chrome/Win10 browser some of the padding on badges might need a little adjustment.

• Is there a plan to utilize the Screen help?

• This looks like a modernized admin page, I wonder if the admin pages will go in this direction? The navigational tabs looks different from the usual ones we see e.g. here /wp-admin/nav-menus.php. Will the ticket be tagged with needs-design-feedback or has it been reviewed by the design team on GitHub?

• Extra left/right padding is needed on the Site's Info description on mobile.

• Is the plan to add the tests before shipping?

[Clorith]

We can do docblocks and minor style adjustments post-beta still, so that's not a problem.

Bulk open/close toggles for the accordions isn't planned, we might be able to look into this in a future iterations, but for now there's no immediate plan for it, ideally you'll be tackling one issue at a time if there's anything that needs addressing.

No plans to utilize the screen help here, there's not really any elements that need the screen help.

It is a modernized admin layout, it's already been handled by the design team, they've had input on it and have been the ones to create this in the first place. We may experiment with further iterations of admin pages taking on a similar look, but nothing definitive at this time, the different between this page, which is more stylized, and many other settings pages is that this is a page you are likely to revisit in the future to maintain a healthy site and keep checking that no problems have occurred (or when we introduce any new checks for example).
The styling is in a way mirrored form the Gutenberg project, and does not use what I would classify as "old school" accordions that use jQuery UI if I recall correctly.

I'm not sure what tests you're referring to here, so I'd need a bit more information before I can answer that question.

[birgire]

Thanks for the quick reply @Clorith and info.

Regarding the tests I see that they will not be included for now (I missed that in my first read):

Some tests and features that exist in the plugin are not quite ready for core inclusion, and have been >dropped from this patch, but may be introduced in future iterations with later versions.

[Clorith]

46573.2.patch​ makes some adjustments for CSS due to core having a slightly smaller font than what the plugin uses, and also removes a debug field we've seen can cause a timeout on some sites, we'll iterate on this one in the plugin and then push it in a later version.

[xkon]

Making a really quick pass here without going into the patch itself. I see that when at the Info tab the Dashboard -> Site Health menu is closed ( most likely due to different file etc ). I think it would be good to have a way on keeping the menu while viewing any "tab".

[Clorith]

Ahh, valid point, we can sort that out quite easily, let me patch that up right away in 46573.3.patch​

[birgire]

ps: I noticed that @since, @param, @return are missing on the new health_check_site_status_tests filter and inline docs referencing on existing ones, e.g. for allow_dev_auto_core_updates.

[xkon]

Awesome @Clorith thanks for that! The site-health-info.php needs also updating to it's links towards site-health.php as well :). They currently link back to the "original" 2 pages so the menu is getting lost again ( to replicate go into the Info tab and then click Status again ).

As we've talked on slack ( mentioning here so it won't be misunderstood also ), I'll be going through files mentioning things that I see so you can get patches up since this is quite big and there's no need for multiple hands to go over these files for now to keep it as clean as possible.

Here goes round 1 ! haha Extra things that I currently see in the patch ( I'll also ask in most cases as I go through each file so haven't looked the overall codebase ):

general things

require_once some parts of core use it with () some others not, not really sure what's the way here I'm a bit confused as it shouldn't have imho.

static calls within the same class, could we use self:: instead? i.e. class-wp-site-health.php - Line 90, again not sure what's the proper way according to what's generally used.

site-info-health.php

Line 20 uses spaces instead of tabs.

Lines 39 to 55 use spaces instead of tabs.

Line 161 the $values should be escaped?

site-health.php

Lines 10 & 11 spaces instead of tabs.

class-wp-site-health.php

Lines 16, 17 & 18 those = could get aligned

Line 90 ( mentioned above for self:: )

Line 170 $_POST['feature'] should be sanitized?

Lines 307, 574 & 970 comment is missing a . :D

Lines 1427 & 1427 sizeof should be count instead ?

We need doc blocks definitely here as pretty much all functions as I see don't have one but can be done in smaller patches etc.

--

I'll be taking looks as much as possible on newer patches as well to at least help this way for now :)

Edit:
The page links was an issue from re-applying the patch on my end. All seems to be in order on that part now!

[Clorith]

46573.4.patch​ should take care of the coding style issues.

The $value entry is escaped further up, that's just the output section.

Docblocks can be added post-merge during beta, which is the thought here to still meet the beta1 window

[xkon]

@Clorith , in class-wp-site-health.php - Line 1348 there's a check that prints out the "Test without plugins" button if the Loopback request fails. Should this button be included here? Doesn't seem like there's any action bound to it at the moment.

[Clorith]

46573.5.patch​ addresses the incorrectly included extra action for loopbacks (this is a feature not planned for core), as it was missed when removing other related actions in the process of generating the core patch.

It also addresses a bug where the default theme (Twenty Nineteen at this time) was being shown twice if it was your active theme, which would be confusing.

[pento]

Thank you for wrangling this patch, @Clorith! My apologies for reviewing it so late in the process. I think we're generally heading in the right direction, but there is some work to be done still. Some of it should probably be done before beta 1, but a lot of it can be put off until after then.

Generally speaking, the biggest issue I ran into was the lack of comments. Both docblocks and inline comments explaining some of the trickier code would've helped a lot.

On the JavaScript:

• I noticed quite a few coding standards issues. Linting it should help.
• Is there a particular reason why things are split into multiple jQuery.ready() calls?
• Because everything is scoped inside jQuery.ready() calls, here's no need for the HC prefix in function names.
• I'm not wild about the HTML template in HCAppendIssue() being generated as a string. Could this be done in a more reusable manner?
• It feels like a lot of the error condition checking in HCRecalculateProgression() is redundant, as the calculations should be taking care of weird values.

For the CSS:

• A lot of these rules seem like there should be equivalent versions already in Core, they're fairly generic.
• Several of the colours don't match those in the ​Design Handbook. Could you double check them all?
• I seem to recall flex layouts being a bit janky in IE11. Could you test that it works correctly?

Into the PHP:

• I'm a bit nervous about the magic Ajax calls which correspond to particular method names. This seems like it could be better done using actions. As we're exposing a lot of sensitive information through these calls, the less magic involved, the better.
• We don't need to check defined( 'ABSPATH' ) at the top of each file.
• All of the esc_html__() and esc_html_e() calls should be replaced with __() and _e(), respectively. We perform appropriate safety checks on strings in GlotPress, instead.
• The PHP and MySQL versions in WP_Site_Health need to be bumped to 5.6 and 5.5, respectively.

This is just written from a code read-through, I skipped auditing each of the tests, but that would be a good thing to spend time on.

[desrosj]

@ianbelanger @earnjam @wpscholar and myself are working through some of these patch issues on a ​branch in the wordpress-develop fork to prevent conflicts while patching.

Tackled so far:

• Ran composer run format to correct coding standards issues on the patch.
• We don't need to check defined( 'ABSPATH' ) at the top of each file.
• All of the esc_html() and esc_html_e() calls should be replaced with () and _e(), respectively. We perform appropriate safety checks on strings in GlotPress, instead.
• The PHP and MySQL versions in WP_Site_Health need to be bumped to 5.6 and 5.5, respectively.

[earnjam]

I think we should change the capability check on this. manage_options works well for single sites, but I don't think that works for the majority of multisite environment setups. In a network, single-site admins would not be able to take action on really any of these recommendations. For example, updating core or uninstalling themes/plugins would both have to be done by a network admin.

Should we adjust it to something like update_core, or add some type of exception for multisite?

[earnjam]

46573.6.diff​ is a collaborative effort from @Clorith, @desrosj, @wpscholar, @ianbelanger and myself from ​a github branch and I think addresses most of the feedback from @pento above.

Still could use some more attention on CSS to ensure we aren't redeclaring styles. I also don't like the repetitive code in the ajax functions now that the magic ajax calls have been removed, but it does prevent those at least.

[pento]

Thank you for working on this, everyone! The patch is looking to be in a much better place.

I'm reviewing it now: if there are minor changes to make, I'll just do them, then commit it.

[knutsp]

Minor:

WP_DEBUG_LOG may be set to file, and a file not in the public folder tree. In that case it's not potentially public, IMHO.

[pento]

In 46573.2.diff​:

• Design improvements from @melchoyce.
• A multitude of string tweaks.
• Proper docblocks everywhere.
• Removed the magic from WP_Site_Health_Auto_Updates::run_tests(). (I definitely used an anonymous function, because we don't have to care about PHP 5.2 anymore.)
• Combined the constant checks in WP_Site_Health_Auto_Updates into one method.
• Altered WP_Site_Health::get_test_php_version() to use wp_check_php_version(), and generally simplified it.
• Removed WP_Site_Health::json_wordpress_version().
• Simplified WP_Site_Health::child_test_php_extension_availability().
• Switched the manage_options permission checks to install_plugins, to more accurately reflect whether a user is capable of finding out this information independently.
• Changed the issue template to use wp.template().
• Moved the menu item to Tools.

Things still to be done, but I think they can all wait until after beta 1.

• HTTPS check shouldn't run for localhost, for which an SSL cert can't be generated.
• REST API failure due to loopback request failing is a weird error message.
• The debug tab is really slow to load.
• Test how WP_Debug_Data::get_directory_size() deals with recursive symlinks.
• WP_Debug_Data::get_database_size() should differentiate between the WordPress database, and all databases on the server.
• From MySQL 5.7, you can get more accurate InnoDB table sizes from INFORMATION_SCHEMA.INNODB_SYS_TABLESPACES: ​https://dev.mysql.com/doc/refman/5.7/en/innodb-sys-tablespaces-table.html
• The values of WP_CONTENT_DIR, WP_PLUGIN_DIR, $upload_dir['basedir'], and get_template_directory() should be shown.
• show_count behaviour needs to be better translatable. Drop-ins should show the count, too.
• Needs better responsive styling.
• WP_Site_Health::get_test_dotorg_communication() should talk to api.wordpress.org.
• Convert all of the actions to buttons. (eg, WP_Site_Health::get_test_php_version())
• Add actions for all tests, so site owners can do something with the information.
• The documentation link in WP_Site_Health::get_test_https_status() should go to a wordpress.org page, or perhaps letsencrypt. Not Cloudflare.
• All text needs reviewing.

[Clorith]

A quick runthrough here, and I notice that the issue template is using escape outputs. For data.description and data.actions we should use unescaped output, as various inline styling and action links are included here, and escaping is handled in the ajax call.

[karmatosed]

I just went through and a few issue struck me to get resolved on top of reported ones (forgive me if they are also known about):

• On first load or page reload for me, the '...' takes a really long time. Shouldn't it remember once it's set? I get this image for at least 10-20 seconds.

• I seem to be seeing raw html tags. For example:

• Overall, there are some hierarchy issues. For example, critical to me needs more stand out styling. Right now visually 'security' stands out way more and that seems to make everything need doing.

• 'Show past tests' isn't a link it's a more loading button, maybe make it styled link that? It seems a weird experience to have it just drop content below.

• I find site info being boxes inside boxes a little weird visually. I'd love to know the reason to just output.

• 'Hide options for copying this information' or 'Show options', doesn't really show options as much as just allow you to copy.

As far as existing issues go:

• Responsiveness: adding the same padding other pages have would be good, I think that's 10px.

Overall, I do feel this feature assumes a lot of knowledge about what it is and does. There is no explanation on the page it just says 'site health'. Whilst that's great if you know what it does, it's not great to assume.

[Clorith]

Looking over the list of remaining issues from @pento first:

The debug tab is a little slow because it still performs some tests, like checking if we can send http requests to wp.org, this is also done by the site health check, so perhaps caching the results here would be an idea to speed things up.

WP_Debug_Data::get_directory_size() was used to get the sizes of the various directories (plugins, themes, uploads), but when @afercia was testing he managed to cause a fatal error fro ma timeout when run on a development setup where npm_modules and vendor directories existed, causing the RecursiveIterator feature to run too long. If we are to re-introduce these as display options, we need a safeguard here, to ensure it doesn't run for too long, or take up too much memory. If either of those cases happen, it should break out early and just provide a "could not determine size" style message for that field.

I agree it would be nice if every single test had an actionable item, ideally one that solved the problem there and then, but for a first iteration I think linking to appropriate wp-admin pages, or documentation, is the best way forward.

We don't have a page that explains https to users as well as they do, yet, but it could be used as inspiration to provide good information in our own article if we were to write one.

Moving along to input from @karmatosed :
The health tests do run every time the site health page, although it is stored as a transient once completed, we could probably leverage this to either not run it on every visit to this page, or introduce a way to re-run things manually.

I totally agree with every item, even recommended ones, feeling urgent because of the badge labeling them as security-related items. Perhaps some of those tests can be classified as something else, or we could also look to use one of the other badge alternatives to not draw as much attention (there's a few styles included that have passed accessibility requirements to choose from)..?

I can't speak for the boxes within boxes, @hedgefield may be able to chime in on the process there, I _think_ the thought was to separate out a long list of often scary information into sections first (accordions) to make the page less daunting, and retain the tabular output of actual data for each section for easier reading (I could be mistaken, hopefully someone will correct me on that).

I'm with you on the assumption bit, the current plugin iteration has an admin notice with a short excerpt on what the pages mean (one on each tab), I don't recommend using that moving forward, as it's a bit tacky thrown inline like that and doesn't work well with the overall view, but perhaps introducing some kind of help icon to show a short description could be the answer here?

[karmatosed]

I'm with you on the assumption bit, the current plugin iteration has an admin notice with a short excerpt on what the pages mean (one on each tab), I don't recommend using that moving forward, as it's a bit tacky thrown inline like that and doesn't work well with the overall view, but perhaps introducing some kind of help icon to show a short description could be the answer here?

I love the idea of a help icon or just something more introductory. I hate to suggest yet another notice but having something show up as a notice on start could help.

[joyously]

Is there a test case for trying this in recovery mode?

[Clorith]

Something simple like that @karmatosed ? (See with-excerpt.PNG​ ), that's just a quick inline thing on an old patch I had, so ignore that styles etc aren't perfect, but essentially a WP admin notice of the info variety in that location is quick, if dirty.

[earnjam]

@Clorith What about just making it a normal paragraph there?

I've also been playing with dashicons on the section headers. Could help call some attention to Critical.

[earnjam]

46573.3.diff​ builds on 46573.2.diff​:

• Add WP_CONTENT_DIR and WP_PLUGIN_DIR to constants
• Change "Show passed tests" toggle/link to a standard button
• Exclude the https test for localhost
• Show count on drop-in plugins
• Switch dotorg communication check to point to api.wordpress.org

Few questions/notes:

1. Where should we add $upload_dir['basedir'], and get_template_directory(), since they aren't constants?
2. WP_Debug_Data::get_directory_size() and WP_Debug_Data::get_database_size() are not actually called anywhere. Probably need to hear from @Clorith where those were supposed to be used.
3. We should add a wordpress.org documentation page about https to point to. LetsEncrypt doesn't really have anything that fits the "what is this and why is it important" questions.

[karmatosed]

Something simple like that @karmatosed ? (See with-excerpt.PNG​ )

Yes, I think it probably could even just a 'one off' notice. Just something that explains what this is and why someone wouldn't be worried as it's helpful.

[earnjam]

Discussed with @xkon and he said the UI that was calling WP_Debug_Data::get_directory_size() and WP_Debug_Data::get_database_size() was removed because of the issues with large directories. So we can reimplement that if we can get some protections in place to prevent timeouts.

[Clorith]

46573.4.diff​ tackles the following:

• Adds a short text what the Site Health is all about.
• Changes the HTTPS information link to one hosted on WordPress.org in HelpHub.
• Adds directory locations and sizes to the debug information, with a max execution checker to break out early and attempt to avoid any timeout messages here.
• Fixes the Site Health tests accidentally having their descriptions and actions escaped.

[pento]

In 46573.5.diff​:

• Riffing a little on @karmatosed's design review, I've changed the "Show copy options" link to just be a "Copy to clipboard" button. The textarea is permanently hidden.
• Improved the non-English language check to only show the "Copy to clipboard (English)" button when its a non-en locale, rather than a non-en_US locale.
• Added extra margin on mobile devices.

There's still plenty of work to be done, but I'm happy to commit this, and continue iterating over the next few weeks.

[pento]

In 44984:

Admin: Introduce the Site Health screens.

The Site Health tool serves two purposes:

• Provide site owners with information to improve the performance, reliability, and security of their site.
• Collect comprehensive debug information about the site.

By encouraging site owners to maintain their site and adhere to modern best practices, we ultimately improve the software hygeine of both the WordPress ecosystem, and the open internet as a whole.

Props Clorith, hedgefield, melchoyce, xkon, karmatosed, jordesign, earnjam, ianbelanger, wpscholar, desrosj, pedromendonca, peterbooker, jcastaneda, garyj, soean, pento, timothyblynjacobs, zodiac1978, dgroddick, garrett-eclipse, netweb, tobifjellner, pixolin, afercia, joedolson, birgire.
See #46573.

[pento]

Converting to task. The remaining issues can be fixed against this ticket, or split out into their own tickets.

[pento]

In 44985:

Admin: Revert [44984].

That was supposed to go into trunk, not the 5.1 branch.

See #46573.

[pento]

In 44986:

Admin: Introduce the Site Health screens.

The Site Health tool serves two purposes:

• Provide site owners with information to improve the performance, reliability, and security of their site.
• Collect comprehensive debug information about the site.

By encouraging site owners to maintain their site and adhere to modern best practices, we ultimately improve the software hygeine of both the WordPress ecosystem, and the open internet as a whole.

Props Clorith, hedgefield, melchoyce, xkon, karmatosed, jordesign, earnjam, ianbelanger, wpscholar, desrosj, pedromendonca, peterbooker, jcastaneda, garyj, soean, pento, timothyblynjacobs, zodiac1978, dgroddick, garrett-eclipse, netweb, tobifjellner, pixolin, afercia, joedolson, birgire.
See #46573.

[Clorith]

Adding a note for the followup task that the copy to clipboard textarea likely needs reverted to a visible field for two reasons:

• Accessibility, as mentioned in ​a GitHub ticket from the accessibility team, it's desired to have this available.
• The clipboard API in browsers has ths as a requirement, the field you are copying must be visible, or else it will not copy anything and you end up with an empty clipboard.

[Clorith]

#39165 was marked as a duplicate.

[afercia]

the copy to clipboard textarea likely needs reverted to a visible field

It also needs to be properly labelled with a visible <label> element associated with for/id attributes.

Re: the visibility issue: right now, this textarea is positioned off-screen. It's still focusable, can be tabbed into, and its entire content gets read out by screen readers:

Putting it in a container hidden with display: none that gets revealed ​as mentioned on GitHub would solve the issue.

[afercia]

The SVG on the progress indicator is focusable in IE 11, thus it's an empty tab stop. This is a known issue in IE 11. In Gutenberg all the SVG icons have the following attributes:

role="img" aria-hidden="true" focusable="false"

The same attributes should be added to this SVG. Let me know if you want me to open a separate ticket.

[afercia]

Related: #46621.

[afercia]

The arrows navigation on the accordion is a bit buggy:

https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/admin/site-health.js?rev=44986&marks=43-49#L42

1
The page scrolls natively when pressing the up/down arrow keys; the function uses keyup so the scrolling can't be prevented. To actually see this behavior the content needs to be long. You can reduce the browser's window height to reproduce.
Either there's the need of a keydown event with the sole purpose of preventing the default (which is meh) or the function should use keydown and prevent the default.

2
Pressing the down arrow moves focus to the last accordion item. Expected: should move focus to the next item. worth noting this jQuery bit:

$( '.health-check-accordion-trigger', $( this ).closest( 'dt' ).nextAll( 'dt' ) ).focus();

actually means:

get all the .health-check-accordion-trigger in the context of all the next dt elements so .focus() runs on the last returned item. There's the need of a .first() after .nextAll()

Instead, the Up arrow works as expected because in this bit of jQuery:

$( '.health-check-accordion-trigger', $( this ).closest( 'dt' ).prevAll( 'dt' ) ).focus();

the last returned element happens to be the one we need.

3
Not sure why toString() is used in e.keyCode.toString(). Worth also reminding when using jQuery ​which should be used instead of keyCode.

[afercia]

46573.7.diff​ removes an unnecessary $( '.health-check-body' ).attr( 'aria-hidden', false ); and the polite parameter from the speak() calls: polite is the default.

[afercia]

In 44988:

Admin: Site Health JavaScript minor clean-up.

See #46573.

[afercia]

This kind of strings:

<span class="issue-count">0</span> <?php _e( 'Critical issues' ); ?>

(note: the count gets updated via JavaScript) are not translatable in languages that have multiple plural forms. This needs to be changed and meet the best practices used in core for proper localization. /Cc @SergeyBiryukov

[karmatosed]

This is coming on, thank you so much for responding to the design feedback given and iterating.

I just tested the latest version and 'Show passed tests' button, doesn't seem to clear when it has loaded. I expected it to maybe not show once loaded.

[xkon]

@karmatosed has a really good point there. To give an alternative thought on that as well just in case it was missed, the Show passed tests button is actually a "toggle" button that Shows & Hides the passed tests. Instead of hiding it completely maybe we could either rename it as to state what it actually does or alternate between Show passed tests Hide passed tests depending on the tests visibility ( not sure which of these would be best for accessibility as well ) ?

[pento]

Replying to Clorith:

Adding a note for the followup task that the copy to clipboard textarea likely needs reverted to a visible field for two reasons:

• Accessibility, as mentioned in ​a GitHub ticket from the accessibility team, it's desired to have this available.

That ticket doesn't specify an accessibility issue, just that it was confusing what was being copied. This could be rectified by modifying the button label (eg, "Copy Report to Clipboard").

• The clipboard API in browsers has ths as a requirement, the field you are copying must be visible, or else it will not copy anything and you end up with an empty clipboard.

As @afercia mentioned, the field is visible, just off-screen, so can be copied. (Or at least, it worked in my brief testing.)

@afercia: if we set aria-hidden="true" tabindex="-1" on the <textarea> elements that exist for the copy button to work, will that be sufficient to hide it from screen readers and keyboard navigation?

[afercia]

1

That ticket doesn't specify an accessibility issue, just that it was confusing what was being copied. This could be rectified by modifying the button label (eg, "Copy Report to Clipboard").

Actually, that GitHub issue makes a clear point:

The main key is giving people an opportunity to review what they're copying as a block prior to copying to clipboard

The actual recommendation is displaying the content of the textarea. I'd say it's not just about accessibility: it's also usability.

2

if we set aria-hidden="true" tabindex="-1" on the <textarea> elements that exist for the copy button to work, will that be sufficient to hide it from screen readers and keyboard navigation?

It would be sufficient for keyboard users, not so much for screen reader users.

• screen reader users navigate content primarily by using arrow navigation provided by ths screen reader: tabindex has no effect
• the actual effects of aria-hidden ​vary across browsers and assistive technologies, especially when used on form elements. For example, Chrome + VoiceOver still read out the textarea content

3
Show passed tests button:

• as mentioned in ​a related GitHub issue, it can't be hidden once activated: that would produce a focus loss
• generally, I'm not in favor of dynamically changing a control's label: the label is the accessible name of a control; its state should be conveyed separately. Potential issue: screen reader users learn there's a control named "xyz". They continue navigation. Then, they look for the previous control and can't find it (or would be in doubt) because the control's name has changed to something else.

[Clorith]

We can change the label to be Toggle passed tests, it then makes sense in both contexts and with an aria-expanded value to indicate if it's visible or not?

[afercia]

There's a Trac ticket about avoiding the term "Toggle", as it's very difficult to translate :) https://core.trac.wordpress.org/ticket/34753

[pento]

46573.8.diff​ removes the textarea elements entirely, using clipboard.js instead, which is also used in the block editor.

[Clorith]

Darn, it seemed like such a simple and good solution... Do we need the prefix, would Passed tests with the aria-expanded attribute be enough? For those not relying on a screen reader the content change would be apparent. It removes a bit of explanation that you should click to see more though...hmmm possibly with the indicator more or less like the accordions above to indicate expanded/collapsed state for those who do see it?

[afercia]

@pento I'd argue that entirely removing the textarea doesn't address the feedback given on the GitHub issue:

The main key is giving people an opportunity to review what they're copying as a block prior to copying to clipboard

@Clorith Passed tests makes sense to me, plenty of similar examples in core:

• Quick edit (not "Show quick edit")
• Upload Plugin (not "Show upload plugin")
• Feature Filter (not "Show Feature Filter")
• Help / Screen Options, etc.

[hedgefield]

Lots of activity on this ticket, I like it! Thanks for chiming in all, some good improvements being made here on the stuff @Clorith and I set up. I had hoped to have received more feedback like this during the weeks we were working on this design and discussing it in the design meetings, but I'll take it.

I am noticing a few things I want to address:

• Great point on adding the introductory copy for each page, I did see it when we started the design phase but spaced on including it in the mockups. I would indeed just present it as plain body text instead of styling it as an admin notice.

• We discussed the colors a few times, perhaps we should tone them down to colors that have less inherit urgency meaning. I'll play around with those before the next design meeting.

• I see the background color changed into grey somewhere along the past week. I can't find that decision mentioned anywhere in this ticket or on the repo, but I heard this was due to standardizing the CSS with the styles in core? I'm all for that technical efficiency, but in this case, I'd like to apply a little more critical thinking. Yes, it may technically be the background color for Gutenberg, but no user would ever see it as that. All you see in the post editor is white, except for the little bit of background of the sidebar. I'd therefore like to argue for making the background white again. It gives a much fresher and calmer look than the harsh grey, and with white it can truly feel like a Gutenberg-alike, instead of a hybrid between the two styles. Plus, this design has been discussed by the design team several times, and the white background was never mentioned as an issue, in fact it was welcomed, so I'd advise we stick with that design. I'm happy to discuss this more, but this decision I'm noticing is a bit dear to me :)

• The info boxes were meant to make the very very long page grokkable indeed, but I think @karmatosed is mostly referring to those boxes having a table in it? We could style that a bit more elegantly yes, starting by not giving the table any margin so it feels more like it is part of the box.

• About the copy-paste thing, do I understand that @pento has gotten the click-to-copy-to-clipboard thing working now? If so that is amazing. To the point of being able to review your copied text - is that really necessary? All the info is technically present on the page, and after copying you can paste it somewhere and then review if you want to remove something from it. I don't think many people scroll through the whole thing to see what all is in there before they copy it, but then again that's only my assumption. I just feel like we are making it harder than it needs to be. The main usecase in my mind is a support engineer asks you to copy that infodump, you want to give someone the quickest way to do so. Feel free to correct me if I'm wrong in that though.

[earnjam]

@hedgefield here is a link to the discussion on background color:
​https://wordpress.slack.com/archives/C02RQBWTW/p1553213550750500

[afercia]

Couple things about the CSS part:

1
The "ellipsis" CSS keyframe animation works only in webkit and Firefox: doesn't work in Edge and IE11. Turns out content isn't one of the CSS ​animatable properties. The fact it works in webkit and Firefox is non-standard and not guaranteed to work in the future. There are alternative CSS techniques ​(a few examples here), but they somehow rely on the font metrics and on animating the width of the "dots" container. I've tried one of them but WordPress uses system fonts so the rendering is different on different operating systems. The width varies and those techniques seem unreliable.

2
The whole CSS uses unnecessary overqualified selectors. For example:
body .health-check-accordion .health-check-accordion-trigger .badge.blue

Seems to me there's no reason to use a so high specificity: this stylesheet is enqueued only in the Site Health pages. According to the WordPress CSS coding standards, specificity should be kept to the lower possible value. Reading ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors two base principles should be applied:

• use specificity with responsibility
• refrain from using over-qualified selectors, div.container can simply be stated as .container

I'd strongly recommend to review all the CSS part as new code merged in WordPress should meet the standards not only for mere compliance but also to illustrate best practices and for educational purposes.

Note: in #46621 I've started reducing specificity in the parts related to that specific issue.

[pento]

I've split the copy button updates off to #46647.

[SergeyBiryukov]

In 45026:

Site Health: Update "Passed tests" button label to match both expanded and collapsed state.

Add aria-controls to declare what ID is being toggled.

Props Clorith, afercia.
Fixes #46663. See #46573.

[afercia]

In 45069:

Accessibility: Remove arrows navigation from the Site Health accordions.

Arrows navigation on accordions is an optional keyboard interaction feature mentioned in the WAI-ARIA Authoring Practices. While it can add some value in some specific cases, it's not so discoverable and it's unlikely users, including assistive technologies users, would really "expect" this kind of interaction.

See #46573.
Fixes #46682.

[desrosj]

#38297 was marked as a duplicate.

[batmoo]

Switched the manage_options permission checks to install_plugins, to more accurately reflect whether a user is capable of finding out this information independently.

Can we allow the capability to be filtered?We restrict the ability to install plugins/themes via wp-admin (they are installed via source code instead) but would still like to allow access to the Site Health section. Right now, it's completely inaccessible (we could hack our way around it but would prefer not to do that).

I suspect other environments with DISALLOW_FILE_MODS enabled will also run into the same issue.

[Clorith]

A valid point, I'd say that if the user doesn't have access to the filesystem as it's locked down, many things become inaccessible to them that the health checker would try to guide them into resolving on their own when they can't, but with plugins and themes already looking to take advantage of the new sections, this has merit.

We're about to hit RC1 later today if all goes as planned, so I've created #46957 which aims to address this for WordPress 5.2.1

[ocean90]

In 45245:

Site Health: Provide context for tab headings to be able to distinguish them from existing strings.

See #46573.

[ocean90]

In 45246:

Site Health: Fix debug data with nested fields for copying.

• Use $debug_data instead of $field['value'] to retrieve the debug data.
• Rename inner variables to avoid overriding the output variable.

Props Clorith, ocean90.
See #46573.

[pento]

Closing this as fixed, everything else is in follow-up tickets.

[desrosj]

Dev note: ​https://make.wordpress.org/core/2019/04/25/site-health-check-in-5-2/

[johnbillion]

In 51297:

Docs: Fix the documentation for the $tests parameter of the site_status_tests filter.

Tests are contained within direct and async properties of this array, not directly in the array itself. This also clarifies the properties that differ between direct and async tests.

See #53399, #46573