WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 4 weeks ago

#37004 closed defect (bug) (fixed)

Html5 / W3C Validation

Reported by: arena Owned by: mihai2u
Milestone: 4.8 Priority: low
Severity: trivial Version:
Component: Administration Keywords: good-first-bug has-screenshots has-patch
Focuses: administration Cc:

Description

I am using Firefox + HTML Validator

This add-on is detecting (using w3c online validation) some html 5 errors.

Mostly on lists (posts, users, search field) etc ...

I think this requires a complete review of wp admin to stick to the w3c standards.

Thank you

Attachments (12)

media-no-errors.png (537.7 KB) - added by mihai2u 12 months ago.
screenshot-media-page-no-validator-error
posts-no-errors.png (550.3 KB) - added by mihai2u 12 months ago.
posts-page-no-validator-errors
37004.diff (4.0 KB) - added by mihai2u 12 months ago.
The patch.
37004.2.diff (257 bytes) - added by topher1kenobe 12 months ago.
37004.3.diff (4.4 KB) - added by michalzuber 12 months ago.
Looks good, but class="colspanchange" should stay; wp-admin/js/common.js is using it.
37004.4.diff (4.3 KB) - added by mihai2u 2 months ago.
w3c-latest-posts.png (321.0 KB) - added by mihai2u 2 months ago.
37004.5.diff (9.9 KB) - added by mihai2u 2 months ago.
37004.6.diff (9.9 KB) - added by mihai2u 7 weeks ago.
37004.7.diff (7.8 KB) - added by mihai2u 5 weeks ago.
37004.8.diff (8.9 KB) - added by afercia 5 weeks ago.
37004.9.diff (8.6 KB) - added by afercia 4 weeks ago.

Download all attachments as: .zip

Change History (49)

#1 @ocean90
12 months ago

  • Version trunk deleted

#2 in reply to: ↑ description @SergeyBiryukov
12 months ago

Replying to arena:

This add-on is detecting (using w3c online validation) some html 5 errors.

Could you give some examples?

#3 @SergeyBiryukov
12 months ago

  • Keywords reporter-feedback added

#5 follow-up: @pento
12 months ago

  • Focuses administration added
  • Keywords needs-patch good-first-bug added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Priority changed from normal to low
  • Severity changed from normal to trivial

Confirmed 3 bugs on the posts list page:

  • <input type="number" step="1" min="1" max="999" class="screen-per-page" name="wp_screen_options[value]" id="edit_post_per_page" maxlength="3" value="20" />

    maxlength="3" is invalid for <input type="number" />. This is a hangover from when it used to be type="text"; max="999" does what we want it to do. The maxlength attribute can be removed.
  • The "View" link for each post has rel="permalink". This is from the WCLR spec, which is obsolete. permalink should be replaced with bookmark, from the HTML5 spec.
  • In the Quick Edit table, there's a colspan="<?php echo $this->get_column_count(); ?>", but it doesn't seem to be necessary. It's a bit of a classic, introduced in [8973], when the Quick Edit table became an entire table, instead of just another row in the post list table. It can go.

#6 follow-up: @ocean90
12 months ago

The maxlength attribute can be removed.

It's still used by browsers which don't support input[type="number"] and fall back to a normal text input.

#7 in reply to: ↑ 6 @pento
12 months ago

Replying to ocean90:

It's still used by browsers which don't support input[type="number"] and fall back to a normal text input.

IE 9 and lower (~1% of browsers), which will fall back to the server-side validation.

Given that it doesn't affect the server, and only has a very minor effect on a UX edge case, I lean towards removing it.

#8 @mihai2u
12 months ago

I confirm this bug is still present, so I'm taking a stab to patch it as my first contributing ticket.

#9 @pento
12 months ago

  • Owner set to mihai2u
  • Status changed from new to assigned

@mihai2u
12 months ago

screenshot-media-page-no-validator-error

@mihai2u
12 months ago

posts-page-no-validator-errors

@mihai2u
12 months ago

The patch.

#10 @mihai2u
12 months ago

  • Keywords has-patch added; needs-patch removed

I've removed the colspan and the class="colspanchange" on that <td> element, since it was no longer necessary without a colspan being present. I confirmed that everything looks good, just like in the attached screenshot even without that class. Actually there are no css styles attached to that class, and it's just common.js changing the count on interactions in a multi-column table. Since these tables only had one single column, it had to be dropped to avoid any unnecessary overhead.

I replaced rel="permalink" by rel="bookmark" in 4 locations, as they all gave validation errors (expected).

The maxlength invalid attribute has been dropped just as well.

#11 @topher1kenobe
12 months ago

Removed colspan tag, changed rel="permalink" to rel="bookmark" and removed maxlength="3".

Not as comprehensive as @mihai2u's I didn't remove the colspanchange class.

@michalzuber
12 months ago

Looks good, but class="colspanchange" should stay; wp-admin/js/common.js is using it.

#12 @mihai2u
2 months ago

  • Keywords has-screenshots added
  • Summary changed from Html5 / W3C Validation to Refreshed

I'm refreshing this, and thanks michalzuber for your contribution of finding the usage of colspanchange inside that .js file.

I've pushed this on Git on top of the latest WP and the fixes still work in achieving HTML validation:
https://github.com/xwp/wordpress-develop/pull/221
( and the github diff: https://github.com/xwp/wordpress-develop/pull/221.diff )

This matches the developments inside the 37004.3.diff from above, and only refreshed. I'll attach the 37004.4.diff matching the git PR as well.

@mihai2u
2 months ago

#13 @mihai2u
2 months ago

  • Summary changed from Refreshed to Html5 / W3C Validation

#14 @mihai2u
2 months ago

  • Keywords needs-testing added

#15 @mihai2u
2 months ago

Received feedback on the GitHub PR from Miina that the Comments list, Users list, Pages list have validations errors.
I'm going to update this in order to cover validation for those pages as well.

#16 @jbpaul17
2 months ago

  • Keywords needs-refresh added; has-patch needs-testing removed

Updated keywords to match current status. Will need to see who can review this and potentially schedule for a planned release once the patch is ready.

#17 @mihai2u
2 months ago

  • Keywords needs-refresh removed

I've done further progress on this task and achieved w3c valid pages in a couple more areas where errors were spotted:
Comments index - removed template colspan updated by JS according to the screen size
Users index - fixed duplicate IDs being present on two buttons
Settings - General - removed inline width on th element, as it's obsolete (width was already defined in CSS)
Settings - General - fixed incorrect positioning of closing span tag around the Custom date format options
Settings - Media - fixed incorrect usage of colspan="2" in the case where there were no other rows/columns in the same table.

I looked into some other w3c errors, and they were related to two things:
1 - mark-up used as a JS template which had table mark-up although it was a single row (posts / tags, posts / categories)
2 - duplicate IDs on the Pages index (the dropdown builder automatically populates the id with the name value passed as a parameter, and there isn't an easy way to make it generate dropdowns with no IDs).

I've updated the github branch with the latest stable WP version, and will upload the corresponding 37004.5.diff for it ( https://github.com/xwp/wordpress-develop/pull/221.diff )

@mihai2u
2 months ago

#18 @swissspidy
2 months ago

  • Keywords has-patch added

#19 @jbpaul17
2 months ago

  • Keywords needs-testing added

Adding Needs Testing keyword to ensure this applies cleanly before looking to milestone the patch.

#20 @jbpaul17
2 months ago

  • Milestone changed from Future Release to 4.8

Milestoning to 4.8 to help get code review on this over next month or so.

#21 @stubgo
7 weeks ago

Tested that the following pages are fixed and without validation errors:

  • Comments
  • Media
  • Posts
  • Plugins
  • Users

#22 @mihai2u
7 weeks ago

  • Keywords needs-testing removed

I've merged in the latest Wordpress master and all applies well without any conflicts:
https://github.com/xwp/wordpress-develop/pull/221
(i'll attach the updated .diff - no changes to the original work)

With this refreshed once more, can it go forward with being merged in @swissspidy ?

@mihai2u
7 weeks ago

#23 @afercia
7 weeks ago

See #40610 for the maxlength attribute.

Also, I think the colspan attribute must stay, since it is used by colSpanChange(), unless I'm missing something.

#24 @mihai2u
7 weeks ago

@afercia

The colspan attributes I removed were always dynamically populated by JS. The colSpanChange() function looks at elements with a "colspanchange" class from what I saw in the code and adds the colspan attribute and value automatically.
The JS does not require the attribute to be present (with an empty or placeholder value), because the attribute gets created with the JS call to set the attribute to a particular value. I verified that the areas where the placeholder colspan was removed from HTML still function well with javascript populating the correct values according to which page / browser size the element appears on.

Regarding your reference to #40610 - this offers the same patch in the same fashion as that ticket there - the removal of the same attribute.
If you'd like me to remove it from this ticket and allow the patch to come through #40610 instead, just ask and I'll accommodate. Browser-based input attribute validation has always been sketchy at best.

Looking forward to getting a checkmark on this ticket, standing by to further support it to completion.

#25 @afercia
7 weeks ago

The JS does not require the attribute to be present

Actually the colSpanChange() function accepts a parameter which is the number to add to the colspan attribute, if present. It does check for the presence of a colspan attribute in the markup:

colSpanChange : function(diff) {
	var $t = $('table').find('.colspanchange'), n;
	if ( !$t.length )
		return;
	n = parseInt( $t.attr('colspan'), 10 ) + diff; <-- check for the table colspan attribute and adds the passed param
	$t.attr('colspan', n.toString()); <-- finally, updates the colspan attribute with the new value
}

#26 @mihai2u
7 weeks ago

Hmm. I'll have to look closer. I haven't looked at the source code specifically, but at how the page looked before and after the colspan removal (and I saw the colspan was added by JS), but didn't really check what number was inside.

Thank you very much for quoting the JS for it.

@mihai2u
5 weeks ago

#27 @mihai2u
5 weeks ago

@afercia

I've refreshed the patch and added back the colspan attributes even if they break validation.
I pondered with the idea of using a data-colspan-diff="" attribute and use that in the JavaScript when present, but I discarded it as I don't think it brings much value. If you find value in going that approach to achieve validation on those particular areas (it's 3 templates receiving the HTML error), I can act on that.

This is the error received: "Table columns in range 2…5 established by element td have no cells beginning in them." -> and that happens because the colspan is part of a template that's used by JS in order to be placed in the proper table which has the columns in that range. Statically speaking, if there's no JS making use of them, they indeed appear as inside a table with not enough columns to make the colspan valid.

I hope all of the remaining code improvements can find their way onto 4.8.1

Thank you for your review.

#28 @afercia
5 weeks ago

@mihai2u thanks for refreshing the patch! To clarify, the colspan attribute is a valid attribute in HTML5, see the spec: https://www.w3.org/TR/html5/tabular-data.html#the-td-element

The quick edit and bulk edit forms work in a very special way. The forms are inside a table that is initially hidden and it's placed at the bottom of the page. This table works as sort of "placeholder" for the quick/bulk edit forms and it's made of two rows with just one column cell (<td>) with a colspan attribute. When activated, the table row containing the form fields gets injected at the top of the list table (bulk edit) or after the table row of a single post (quick edit).

In the list tables, some columns can be optionally displayed or hidden: users can choose which columns to display from the Screen Options panel at the top of the page. That means there's no way to predict how many columns a table has.

So, when the quick/bulk edit table row gets injected and become visible, it needs to know how many columns are currently set to be displayed in the table and update its colspan attribute accordingly, otherwise the layout will break.
Note that users can also hide/display columns while the quick/bulk edit form is open, and when this happens, the JS needs to update on the fly the colspan attribute value.

The validator message is actually different depending on the table you're validating and the initial value of the colspan attribute. Checking the post list table, for me is:
Table columns in range 2…7 established by element td have no cells beginning in them.

That doesn't mean the colspan attribute is not allowed. It just means the validator "sees" a table cell with td colspan="7" so that column is supposed to stretch across seven columns but that table doesn't contain other rows with 7 columns. It's the hidden table at the bottom of the page that works as "placeholder" for the quick/bulk edit rows.

It's a bit complicated to explain. In a few words, I wouldn't be so worried because the "error" the validator sees is in a hidden table used just for printing out a template. It's not available to assistive technologies and doesn't break anything.

#29 @mihai2u
5 weeks ago

Great, thank you for the thorough explanation @afercia
Then I believe we should be moving forward with the other fixes I've made to improve the HTML validity and that would be it for this ticket.

Good first bug? :)

Looking forward to the commit to core and putting this to sleep.

#30 in reply to: ↑ 5 @afercia
5 weeks ago

Replying to pento:

The "View" link for each post has rel="permalink". This is from the WCLR spec, which is obsolete. permalink should be replaced with bookmark

For history: introduced in [480] on 10/26/03 :)

@afercia
5 weeks ago

#31 @afercia
5 weeks ago

Refreshed patch. I'd say to take care of maxlength in #40610.

Spotted a few more things, most notably: $detach_url in the media list table needs to be escaped. Other things:

  • 1 more permalink rel attribute in Multisite
  • missing span in wp-admin/includes/template.php (in the "Attach media" modal)
  • let's make that table row in the Users screen use a td instead of a th, there's no need for a table header there

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


5 weeks ago

#33 @mihai2u
4 weeks ago

Thank you for the update, afercia.
I'll do another refresh which would handle the items you referenced.

#34 @afercia
4 weeks ago

@mihai2u the latest patch should already cover those items.

#35 @mihai2u
4 weeks ago

Thanks for the clarification, afercia. I thought you were asking me to make those updates.
Then it's should be smooth sailing towards getting committed.

@afercia
4 weeks ago

#36 @afercia
4 weeks ago

Turns out I was wrong about $detach_url, the validation error there is about the square brackets, not the ampersand. Also, there's a long standing discussion about the whole concept of "Attach" and "Detach" (see #6820) and there's a chance they will be removed in the future. I'm not comfortable in trying to fix that link now that we're in beta 2, so I'm leaving it out.

#37 @afercia
4 weeks ago

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

In 40823:

Administration: Fix some HTML validation errors.

Fixes some minor HTML issues in the admin and, most notably, changes the rel
attribute used in the List Tables from rel="permalink" to rel="bookmark".

Props mihai2u, pento, arena, topher1kenobe, michalzuber, stubgo.
Fixes #37004.

Note: See TracTickets for help on using tickets.