Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#37968 closed defect (bug) (invalid)

HTML validation errors in admin tool

Reported by: mdgl's profile mdgl Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 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 8 years ago.
37968-akismet.diff (2.0 KB) - added by mdgl 8 years ago.
37968-nav-menus.diff (1.0 KB) - added by mdgl 8 years ago.
37968-nav-menu-alt.diff (1.0 KB) - added by mdgl 8 years ago.
Alternative patch for nav menus issue

Download all attachments as: .zip

Change History (18)

@mdgl
8 years ago

@mdgl
8 years ago

#1 @SergeyBiryukov
8 years ago

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

#2 @mdgl
8 years 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.

@mdgl
8 years ago

#3 @mdgl
8 years 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
8 years ago

Alternative patch for nav menus issue

#4 @mdgl
8 years 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
8 years 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
8 years ago

  • Keywords has-patch added

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


8 years ago

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


8 years ago

#9 @helen
8 years 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.


8 years ago

#11 @helen
8 years ago

  • Keywords reporter-feedback added

#12 @mdgl
8 years 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
8 years 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.

#14 @desrosj
6 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from reviewing to closed

Hi @mdgl,

Thank you for all the work you put into this ticket. I looked into all of the issues except Akismet (you should report that separately to Akismet support if it's still occurring).

The other mismatched The HTML warnings you detailed all appear to have been fixed through efforts on other tickets. The missing </li> in the menu walker appears to be intentional, though, as the <li>s are closed when end_el() method inherited from the parent class.

I am going to close this out, but if you are still able to identify invalid HTML, please feel free to reopen with more details.

Note: See TracTickets for help on using tickets.