WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 19 months ago

#37968 reviewing defect (bug)

HTML validation errors in admin tool

Reported by: mdgl Owned by: SergeyBiryukov
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

The admin tool seems to cause a few HTML validation errors when displaying the following screens:

  • Media - Library
  • Settings - General
  • Settings - Akismet

These were spotted using the developer console in Microsoft Edge and are all related to improperly closed tags. Patches incoming shortly, although I'm not sure whether this is the right place to report issues with Akismet.

Attachments (4)

37968.diff (3.2 KB) - added by mdgl 22 months ago.
37968-akismet.diff (2.0 KB) - added by mdgl 22 months ago.
37968-nav-menus.diff (1.0 KB) - added by mdgl 21 months ago.
37968-nav-menu-alt.diff (1.0 KB) - added by mdgl 21 months ago.
Alternative patch for nav menus issue

Download all attachments as: .zip

Change History (17)

@mdgl
22 months ago

@mdgl
22 months ago

#1 @SergeyBiryukov
22 months ago

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

#2 @mdgl
21 months ago

I found another similar validation problem in the Appearance - Menus screen. It's tempting to ask who tests this stuff, but in fact modern browsers are very relaxed with HTML parsing and seem to correct obvious errors automatically, so in practice this kind of bug is becoming very difficult to spot!

Class Walker_Nav_Menu_Edit is used to populate the "Menu Structure" panel with the menu to be edited. If the menu contains nested items, however the current code attempts to output a nested list of just <li> elements which is not valid HTML without an intervening <ul> (or <ol>).

The solution I think is to follow the example of class Walker_Nav_Menu_Checklist and override the start_lvl() and end_lvl() methods to output the necessary additional <ul> and </ul> markup. See the forthcoming patch for a suggestion. Tab indents and closing tag comments could perhaps be further improved.

Unfortunately, this change also causes the display of the nested menu items to be indented rather more than previously. Some CSS changes are probably required as well, but I'm not sure of the best place to do that - the current CSS seems rather over-specified, perhaps as a result of trying to work around the invalid markup.

#3 @mdgl
21 months ago

Actually, after thinking about this a little more, another way to fix the issue with the menu edit screen would be to properly flatten the list, outputting a complete <li> and </li> element within method start_el() and overriding end_el() to do nothing. Not sure whether that would be preferable - it is less "semantic" but might fit with the current CSS a bit better.

@mdgl
21 months ago

Alternative patch for nav menus issue

#4 @mdgl
21 months ago

Alternative patch for nav menu issue outputs a properly flattened list which works with the current CSS but is perhaps less "correct" than creating a nested list.

#5 @mdgl
21 months ago

Further investigation into the nav menus issue reveals that the JavaScript associated with the edit screen assumes/requires a flattened list so that it is easier to promote/demote items in the hierarchy by just changing class names. I think the alternative patch (37968-nav-menu-alt.diff) is therefore the one to use to fix the Appearance - Menus screen.

#6 @mdgl
21 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


20 months ago

#9 @helen
20 months ago

  • Milestone changed from 4.7 to Awaiting Review

@mdgl - I'm sorry this hasn't gotten more eyes. Unfortunately need to punt this from 4.7, as this doesn't appear to be critical and we're into beta now without any activity here for quite some time.

Just for future reference - what errors are you getting exactly, and what are the consequences of those errors, if any?

This ticket was mentioned in Slack in #core by helen. View the logs.


19 months ago

#11 @helen
19 months ago

  • Keywords reporter-feedback added

#12 @mdgl
19 months ago

  • Keywords reporter-feedback removed

Sorry for the delay but I thought we were giving up on this ticket. A description of the errors appearing in the Microsoft Edge console log is included below.

There don't appear to be any serious consequences of these errors, perhaps just the "embarrassment factor" because we are generating invalid HTML! It is possible though, that other browsers will behave differently when parsing the invalid markup and this might result in different styling being applied to various elements, depending on how they are interpreted by the browser. HTML5 does define some rules for how browsers are supposed to behave when presented with invalid markup, so this may not be a major issue, but I haven't had time to check.

Media -> Library

When displaying the media library screen in list mode, the following error appears because of an unclosed <span> element.

HTML1508: Unmatched end tag.
upload.php (549,138)

Settings -> General

The following errors appear on the general settings screen because of improperly nested <span> and <input> elements (in particular the <span> is opened inside the <input> element but closed outside).

HTML1508: Unmatched end tag.
options-general.php (829,266)
HTML1512: Unmatched end tag.
options-general.php (829,469)
HTML1508: Unmatched end tag.
options-general.php (840,248)
HTML1512: Unmatched end tag.
options-general.php (840,446)

Settings -> Akismet

There are two issues with the Akismet settings screen. Firstly, Akismet attempts to self-close a <td> element which (perhaps surprisingly) is not allowed in HTML5. Secondly, there is an unclosed <span> element.

HTML1500: Tag cannot be self-closing. Use an explicit closing tag.
options-general.php (250,13)
HTML1508: Unmatched end tag.
options-general.php (273,184)
HTML1500: Tag cannot be self-closing. Use an explicit closing tag.
options-general.php (303,12)
HTML1500: Tag cannot be self-closing. Use an explicit closing tag.
options-general.php (310,12)

Appearance -> Menus

Errors similar to the following appear if you use the appearance menus screen to display a menu that contains nested/hierarchical entries. The exact number of errors and their line numbers will depend on the menu structure. These errors occur because the <li> elements are not nested properly (see comment:2 above).

HTML1509: Unmatched end tag.
nav-menus.php (1867,1)
HTML1509: Unmatched end tag.
nav-menus.php (1868,1)
HTML1509: Unmatched end tag.
nav-menus.php (2108,1)
HTML1509: Unmatched end tag.
nav-menus.php (2109,1)

#13 @helen
19 months ago

  • Version trunk deleted

Got it, thanks. Clearing the version since this isn't new to trunk, unfortunately I can't put time into actually looking at these actual instances and whether it makes sense to fix things at the moment, but if anybody wants to look at this and make a decision that would be appreciate.

Note: See TracTickets for help on using tickets.