Make WordPress Core

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's profile afercia Owned by: sergeybiryukov's profile 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 afercia)

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:

http://cldup.com/w5oC9shd5I.png

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:

http://cldup.com/4Dy8ncZKeS.png

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)

48948.1.diff (640 bytes) - added by maxpertici 4 years ago.
Replace <p> tag with <div> tag and tag conditional statement on data.actions

Download all attachments as: .zip

Change History (9)

#1 @afercia
5 years ago

There are also cases where a test may want to pass multiple actions, as core itself does:

http://cldup.com/y2OYCMwvsj.png

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.

#2 @afercia
5 years ago

  • Description modified (diff)

@maxpertici
4 years ago

Replace <p> tag with <div> tag and tag conditional statement on data.actions

#3 @maxpertici
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.

#4 @maxpertici
4 years ago

  • Keywords has-patch added; needs-patch removed

#5 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @SergeyBiryukov
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.

#7 @SergeyBiryukov
4 years ago

In 47528:

Site Health: Correct markup in ::get_test_php_extensions() and ::get_test_background_updates() description.

These tests output an unordered list, which doesn't need to be wrapped in a paragraph tag.

Additionally, pass an empty string as an actions parameter in ::get_test_php_default_timezone(), for consistency with other tests.

Props afercia.
See #48948.

#8 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47529:

Site Health: Remove paragraph tag from the actions container in issue template.

Most of the tests pass content that is already wrapped in a paragraph or list tags, thus producing nested paragraphs or invalid markup.

Additionally, don't output an empty <div> tag if the test does not provide any actions.

Props maxpertici, afercia.
Fixes #48948.

Note: See TracTickets for help on using tickets.