Opened 5 years ago
Closed 4 years ago
#48948 closed defect (bug) (fixed)
Fix the Site Health template to produce valid HTML
Reported by: | afercia | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | 5.2 |
Component: | Site Health | Keywords: | has-screenshots has-patch |
Focuses: | Cc: |
Description (last modified by )
Working on an external project that adds tests to Site Health, I noticed the JavaScript issue template produces invalid HTML, depending also on the passed content.
See the template, specifically the parts related to the issue description and actions:
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/site-health.php?rev=46586&marks=151-156#L143
<script id="tmpl-health-check-issue" type="text/template"> ... <div id="health-check-accordion-block-{{ data.test }}" class="health-check-accordion-panel" hidden="hidden"> {{{ data.description }}} <div class="actions"> <p class="button-container">{{{ data.actions }}}</p> </div> </div> </script>
Description
It's not wrapped within other HTML elements other than the main <div>
thus there are no nesting problems and any HTML can be used.
However, some WordPress tests pass content that's actually an unordered list within a paragraph. See for example https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-site-health.php?rev=46797&marks=1640-1649#L1638
Browsers are tolerant and their parsers will try to produce well-formed HTML thus actually rendering empty paragraphs:
This is responsibility of some specific WordPress tests that output invalid HTML and should be fixed in the tests.
Actions
The template wraps the actions within a paragraph. This assumes that the content passed for the actions needs to be content that can be nested within a paragraph, for example a simple link. However, most of the tests pass content that is already wrapped in a paragraph thus producing nested paragraphs.
Also, I can't seem to be able to find any documentation on the recommended HTML that should be passed in order do have valid HTML. To help plugin authors to produce valid HTML, it would be best to:
- either document what the recommended markup is
- or just replace the template paragraph with a
<div>
thus allowing any HTML
The second option would make plugin authors responsible for the output markup: core should encourage authors to produce valid semantic HTML with good examples
Going through the WordPress tests, the content passed for the actions is inconsistent:
1
In a couple cases, the passed string is a link: it will be correctly rendered within the paragraph rendered by the template. See for example https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-site-health.php?rev=46797&marks=239-243#L238
2
More often, the actions HTML is already wrapped within a paragraph. It will be rendered within the paragraph from the template (nested paragraphs). Also in this case, the browsers parsers will try to "fix" the HTML thus producing empty paragraphs.
Also, worth noting the action is actually rendered within a paragraph that doesn't have the button-container
class so this CSS class can't be used for styling:
3
Sometimes, the action is an empty string because there's no action to take or because the test passed: an empty paragraph will be rendered. In these cases, it would be better to not render the entire actions <div>
. A simple check for data.actions
may help.
4
Seems to me the get_test_php_default_timezone
test is the only one that doesn't set actions
in the result array, not sure why. Within the template data.actions
will be undefined
. I guess it should pass an empty string for consistency.
Related (in a way): #47210
Attachments (1)
Change History (9)
#3
@
4 years ago
Hello, I propose a patch with a replacement of html tag (p-> div) and a basic condition in the template on data.actions as mentioned in the ticket.
#5
@
4 years ago
- Milestone changed from Awaiting Review to 5.5
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#6
@
4 years ago
It looks like <p class="button-container">
here was copied from wp_dashboard_php_nag().
Since this class is unused in core and Site Health actions only contain links, not buttons, I think there's currently no need to replace the <p>
tag with <div>
, it can just be removed.
There are also cases where a test may want to pass multiple actions, as core itself does:
I'd tend to think plugin authors should be allowed to make a decision on the HTML they want to use. For example, they may want to use a paragraph for each action or maybe an unordered list to group the actions or any other HTML. Instead, the core template wraps the actions within a paragraph by default, which seems to me an assumption and limitation that should be removed.