Make WordPress Core

Opened 14 years ago

Closed 12 years ago

Last modified 5 years ago

#15080 closed enhancement (fixed)

Comment Form Should use HTML5 input types for better accessibility

Reported by: jorbin's profile jorbin Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.0
Component: Accessibility Keywords: has-patch twentythirteen commit
Focuses: Cc:

Description

Browsers that don't support url and email will fall back to text. Good browsers (and accessibility user agents) will make better use.

Attachments (11)

a11ydc.patch (1.5 KB) - added by jorbin 14 years ago.
15080.patch (2.1 KB) - added by jorbin 12 years ago.
15080.updated.patch (2.1 KB) - added by lessbloat 12 years ago.
15080.args.diff (4.1 KB) - added by jorbin 12 years ago.
15080.args.2.diff (4.1 KB) - added by jorbin 12 years ago.
15080.3.diff (3.5 KB) - added by georgestephanis 12 years ago.
15080.4.diff (2.8 KB) - added by obenland 12 years ago.
Simpler approach without input builder
15080.5.diff (2.9 KB) - added by obenland 12 years ago.
Simpler approach without input builder, but with empty pattern attributes
15080.6.diff (2.9 KB) - added by georgestephanis 12 years ago.
15080.7.diff (5.2 KB) - added by sscovil 12 years ago.
Wrapped comment form help text in label tags for accessibility.
15080.8.diff (5.1 KB) - added by sscovil 12 years ago.
Removed 'for' attributes from new labels added for accessibility in 15080.7.

Download all attachments as: .zip

Change History (68)

@jorbin
14 years ago

#1 @nacin
14 years ago

Also, type="search" in get_search_form(). Think jorbin is opening a new ticket.

#2 @demetris
14 years ago

I looked into this a few weeks ago, saw some things I did not like and decided to forget about it for now.

I do not recollect exactly what the things I did not like were, but something that stands out is that more and more browsers(1) start acting on these input types. One important thing they do is validation. They do that in different ways, and sometimes things go very wrong:

http://getsatisfaction.com/huffduffer/topics/opera_not_recognising_eu_on_input_type_url

:-D

Also, I am not sure what the consensus is about these input types among accessibility folks.

So, I would not like to see this added to default template functions until we have done some proper research.

For entertaining and impatient folks, changing the input types of the new comment form can be done easily by attaching a filter to comment_form_default_fields:

function my_html5_comment_form($i)
{
    $i['email'] = str_replace('"text"', '"email"', $i['email']);
    $i['url']   = str_replace('"text"', '"url"', $i['url']);
    
    return $i;
}

(1) Now Firefox too. It was committed in August. The current beta of Firefox 4 should have it.

#3 @demetris
14 years ago

Another problematic aspect I forgot to mention:

These input types are invalid in XHTML 1.0 and in HTML 4.

#4 follow-up: @jorbin
14 years ago

I tested in opera 10.62 and both .in and .eu domains work fine. I think that was an old bug.

http://webaim.org/blog/future-web-accessibility-new-input-types-in-html5/ has some info on how these improve accessibility.

Also, the iphone does some very cool things with the type that we would now be enabling: http://diveintohtml5.org/forms.html

Sites that want to use xhtml or html4 should be required to filter the content. Twenty Ten is html5.

#5 in reply to: ↑ 4 @demetris
14 years ago

Replying to jorbin:

I tested in opera 10.62 and both .in and .eu domains work fine. I think that was an old bug.

It is not that old (it was reported 3 months ago) and, in any case, I only mentioned it as an example. There are more issues and strange bits of behaviour. If you play a bit with desktop browsers that understand these types, you’ll see that current implementations are not such that you would want to trust them with your user’s experience. This thing is new and we should expect issues to keep popping up while it is new.

http://webaim.org/blog/future-web-accessibility-new-input-types-in-html5/ has some info on how these improve accessibility.

The WebAIM article does not discuss the accessibility of existing implementations. It just talks about what user agents could or might do in the future with the new types.

Also, the iphone does some very cool things with the type that we would now be enabling: http://diveintohtml5.org/forms.html

I know about the iPhone keyboard adjustment and I agree that it is a smart and useful thing, but mobile themes can, and do, take care of that. Both WPtouch and WPtouch Pro, for example, use the new input types. WPtouch since March 2010; WPtouch Pro since its first release.

Sites that want to use xhtml or html4 should be required to filter the content. Twenty Ten is html5.

comment_form is not a Twenty Ten function. It is a core function meant for all themes.

Expecting themes to use filters so that they generate valid XHTML 1 or HTML 4 does not seem like a sound approach to me.

#6 @westi
14 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

This is a really cool idea - not sure we should do this at this point in the 3.1 cycle though

#7 follow-up: @alex-ye
14 years ago

HTML 5 in the search form ! That sounds great but what about the complitly with browsers ??

#8 in reply to: ↑ 7 @GaryJ
14 years ago

Replying to alex-ye:

HTML 5 in the search form ! That sounds great but what about the complitly with browsers ??

If browsers don't recognise the type value attribute on an input element, they default to "text".

#9 @Elpie
14 years ago

The submitted patch is not valid HTML5 but irregardless, the HTML5 specification is still under development, browser support is not universal, and the core should not be imposing a particular DOCTYPE on users. Anyone who wants to use HTML5 now should know enough to be able to write their own form.

See also: #15081

Last edited 14 years ago by Elpie (previous) (diff)

#10 @markjaquith
14 years ago

the HTML5 specification is still under development

It's a living spec. http://blog.whatwg.org/html-is-the-new-html5

There isn't going to be some magic moment where everyone is officially encouraged to start using new HTML features. After Firefox 4 and Internet Explorer 9 are released, things are going to get interesting. We can make these calls on a case-by-case basis.

#11 @nacin
13 years ago

  • Keywords changed from has-patch, 3.2-early to has-patch 3.2-early

Let's do this.

#12 @husobj
12 years ago

  • Cc ben@… added

#13 @mdawaffe
12 years ago

Patch still applies cleanly with offset.

#14 @nacin
12 years ago

  • Keywords commit added
  • Milestone changed from Future Release to 3.5

I think the main reason why there's been a hesitation is that an input[type=text] selector will no longer work. Worth a chance, though.

@jorbin
12 years ago

#15 @jorbin
12 years ago

Absolutely worth a chance.

I think the hesitations was around validating differences in browsers. Updated patch eliminates that.

#16 @bpetty
12 years ago

  • Cc bpetty added

See #20579 for a fix for this along with default HTML5 styles in the appropriate bundled themes (Twenty Eleven and Twenty Twelve) rather than in core.

#17 @bpetty
12 years ago

Out of curiosity, how well does 15080.patch work when the theme in use does not use an HTML5 doctype in various browsers? Need to consider that various themes could be using any of HTML4, XHTML, or HTML5 assuming this change is going to be globally done. Also consider that this would technically be breaking compatibility for older themes possibly.

I'd love for this to be a global change like the patch here, but in the interest of compatibility, this might still be best handled per theme as implemented in older patches on #20579.

#18 @helenyhou
12 years ago

  • Keywords needs-refresh added; 3.2-early removed

#19 @lessbloat
12 years ago

Tested this in IE7 (win), and Chrome (OSX). IE7 worked as expected. Chrome showed up as:

http://f.cl.ly/items/2c0E313j1N160j2t3a01/css-type-email-or-url.jpg

Though I imagine this is fixed in the theme specific patch. 15080.updated.patch looks good to me.

#20 @helenyhou
12 years ago

  • Keywords commit needs-refresh removed

#21 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

15080.updated.patch works as-expected on iPad on current revision of Twenty Twelve, though I suppose there may be a bundled theme ticket to match.

#22 @helenyhou
12 years ago

#20579 for bundled themes styling. Not sure about dropping it in unless bundled themes are also updated, though.

#23 @sabreuse
12 years ago

Updated patch works, and the latest patch on #20579 fixes the styling bug identified in comment 19. +1 to both.

#24 @georgestephanis
12 years ago

I like this conceptually, but some third party themes may be styling the comment form how twentyten and twentyeleven currently do -- input[type=text] -- which would break if we did this, as the css rules aren't in place to handle it.

Personally, I'd like to see the theme itself dictate a transition to html5 input types.

#25 @georgestephanis
12 years ago

Revised patch attached after talking to jorbin

#26 @jorbin
12 years ago

conversation - https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-10-26&sort=asc#m480067

tl;dr: making it easier for themes to implement this makes more sense then potentially breaking themes using css on input[type=text]

#27 @nacin
12 years ago

  • Keywords 3.6-early twentythirteen added
  • Milestone changed from 3.5 to Future Release

Too late to introduce something new. This is a good idea, though. Let's leverage this in the Twenty Thirteen theme.

#28 @retlehs
12 years ago

  • Cc retlehs added

#29 @jorbin
12 years ago

So a problem I noticed with 15080.args.diff from georgestephanis is that we don't add the pattern attribute to the html5 fields or the novalidate attribute to the form field (see earlier discussion on concerns with browsers auto validating). I've added that along with also adding the required property when we are in html5 comments and the field is required.

@jorbin
12 years ago

@jorbin
12 years ago

#30 @lancewillett
12 years ago

  • Keywords 3.6-early removed
  • Milestone changed from Future Release to 3.6

I moved #15080 into 3.6, we really need to get this working so that Twenty Thirteen can not have extra functions for search inputs (and comments).

Noting Nacin's comments regarding adding in the new argument to make things easier for back compat: http://core.trac.wordpress.org/ticket/15081#comment:7

And also #22005 for good measure.

All three tickets share the same principle: push core to output better, future-thinking markup while not breaking older themes.

#31 @SergeyBiryukov
12 years ago

#23614 was marked as a duplicate.

#32 @azizur
12 years ago

  • Cc azizur added

#33 follow-up: @georgestephanis
12 years ago

Just added 15080.3.diff -- a retooling of Jorbin's last patch. Still uses the input_builder function (slightly tweaked) as well as tidies up some of the arguments, whitespace, misspellings, and such.

I see us migrating to a form building class down the road, and at that point, the input_builder function (possibly better namespaces wp_input_builder) can just map to a class method. And if we don't -- hey -- at least we're not pining for the fjords in the meanwhile.

@obenland
12 years ago

Simpler approach without input builder

#34 follow-up: @obenland
12 years ago

In the interest of getting HTML5 support in, I'd like to propose a simpler solution. It follows the example $req sets, checks whether html5 was specified as the format, and outputs the corresponding input types. I moved away from adding the pattern parameter since we don't want to validate the form anyway.

Feedback welcome!

Last edited 12 years ago by lancewillett (previous) (diff)

#35 @lancewillett
12 years ago

Added #23702 to update Twenty Thirteen once this core change is committed.

#36 in reply to: ↑ 34 @jorbin
12 years ago

Replying to obenland:

In the interest of getting HTML5 support in, I'd like to propose a simpler solution. It follows the example $req sets, checks whether html5 was specified as the format, and outputs the corresponding input types. I moved away from adding the pattern parameter since we don't want to validate the form anyway.

Feedback welcome!

I think the The reason for the use of the null pattern was that some browsers didn't support the novalidate attribute on the form and attempted to do validation. if I had to guess, it was a version of Opera. According to a cursory glance of the internet, it is fine in Opera 11.

Otherwise this patch looks like the best route until we can get a proper form builder in.

#37 @esmi
12 years ago

With regard to HTML5, can I just point out that not all assistive technology supports HTML5 yet - which is perfectly reasonable considering it's still only a draft spec. So any patch needs to ensure that all aspects of the form are still accessible to these users.

@obenland
12 years ago

Simpler approach without input builder, but with empty pattern attributes

#38 @obenland
12 years ago

FWIW, according to this document novalidate is supported by Opera 10.6+ while pattern is supported by Opera 11+. We should be good either way.

Esmi, are all aspects of the form still accessible in the proposed patches?

#39 @sscovil
12 years ago

  • Cc sscovil@… added

#40 @esmi
12 years ago

Can you give us a day or 2. We (as in the a11y group) are looking at it now with a view to perhaps submitting a revised patch.

#41 in reply to: ↑ 33 @SergeyBiryukov
12 years ago

Replying to georgestephanis:

I see us migrating to a form building class down the road, and at that point, the input_builder function (possibly better namespaces wp_input_builder) can just map to a class method.

Related: #23237

#42 @SergeyBiryukov
12 years ago

In 15080.5.diff, the default format value in line 1547 should probably be 'xhtml', for consistency with [23667].

#43 @georgestephanis
12 years ago

Changed default from blank to xhtml as per Sergey in 15080.6.diff

#44 @SergeyBiryukov
12 years ago

  • Keywords commit added

#45 @sscovil
12 years ago

Just making an accessibility change now...will upload patch momentarily.

@sscovil
12 years ago

Wrapped comment form help text in label tags for accessibility.

#46 @sscovil
12 years ago

The old comment form was outputting help text inside <p> tags, as seen here: http://pastebin.com/SgMaD22e

According to @joedolson: "Having paragraphs inside a form can require a screen reader user to move back and forth between modes to read all text."

The new patch resolves this issue by wrapping form help text in <label> tags, as seen here: http://pastebin.com/yzpBBTD4

Last edited 12 years ago by sscovil (previous) (diff)

#47 @obenland
12 years ago

Hi sscovil, the paragraphs are still inside the form, I'm not sure I follow? In instances where these help texts would be used, there will be no corresponding form element to bind it to. What are 'author' and 'name'?

I want to add that many themes will style labels inside the comment form with a selector like #commentform label, adding labels in places where previously weren't any will break backwards compatibility.

#48 @sscovil
12 years ago

Hi Konstantin, thanks for the response. Having paragraph tags inside the form is fine, as long as the text within them is wrapped in a label or input tag. I'm not sure if using a span tag would have the same effect, but I can look into it...

On second thought, I suppose using a span tag presents the same potential compatibility issue with themes. Unfortunately, as it stands, the comment form itself has a compatibility issue with screen readers...so I'm open to suggestions.

I will be meeting with the WP Accessibility group tomorrow and can bring any questions posted here to some folks who rely on screen readers and are more familiar with SR web form issues.

Last edited 12 years ago by sscovil (previous) (diff)

@sscovil
12 years ago

Removed 'for' attributes from new labels added for accessibility in 15080.7.

#49 @sscovil
12 years ago

This patch is a bit cleaner; I'm not sure we would need a for attribute on those new labels, and omitting them prevents the styling from getting mucked up in Twenty Thirteen.

Here is the updated form output, both for logged-in and anonymous users: http://pastebin.com/Y885JTU0

Last edited 12 years ago by sscovil (previous) (diff)

#50 @georgestephanis
12 years ago

Yeah, but semantically, it's really bad practice. They're not labels for anything. Labels are meant to be paired to a form control.

http://www.w3.org/TR/html-markup/label.html

#51 @sscovil
12 years ago

I agree, it's still a hack. I'll dig deeper tomorrow and find a better solution.

#52 follow-up: @jorbin
12 years ago

Adding labels seems like an entirely different conversation then the one at hand. Also one that has similar backwards compatibility concerns as were expressed above. I think we should open a separate ticket to discuss that and commit georgestephanis's most recent patch 15080.6.diff

#53 @esmi
12 years ago

semantically, it's really bad practice

No - it isn't. The spec actually says" The label element may contain at most one descendant input element". The keywords being "at least one". It is perfectly valid to have more than 1 label attached to a form control. The "Your email address..." line applies directly to the email input. The "your are logged in as" line should, logically, be associated to the Name input whilst the allowed tags content needs to be associated with the textarea.

All text within form tags must be within labels - otherwise SR users will be forced to make 2 passes (one on forms mode, the other in virtual cursor/reading mode). That, of course, assumes that the users are even aware that there is plain text in the form. Most will have no idea that this additional, informational, content is present as most SRs enter forms mode automatically on hitting the form tag or the first form control.

Version 0, edited 12 years ago by esmi (next)

#54 @sscovil
12 years ago

@esmi: What @georgestephanis was referring to was my latest patch (15080.8.diff), in which I removed the for attribute from the labels. That is semantically wrong.

The issue raised by @obenland was that, by moving the helper text into labels, this could mess with the styling in some themes -- it actually does, in Twenty Thirteen, due to the following CSS:

#commentform label[for="author"], #commentform label[for="email"], #commentform label[for="url"], #commentform label[for="comment"] { float: left; padding: 5px 0; width: 100px; }

I think this is an important issue to resolve, but I also agree with @jorbin that it is beyond the scope of this particular ticket.

#55 in reply to: ↑ 52 @lancewillett
12 years ago

Replying to jorbin:

I think we should open a separate ticket to discuss that and commit georgestephanis's most recent patch 15080.6.diff

+1. Yes, please. We need to wrap up this ticket for 3.6, and we can open new tickets to keep taking more steps for next release.

#56 @markjaquith
12 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 23689:

Add the ability to use HTML5 input types in the comment form.

props jorbin, georgestephanis, obenland. fixes #15080

#57 @westonruter
5 years ago

Are the concerns about client-side validation that prompted the inclusion of the novalidate attribute on the comments form still valid? Please weigh in on #47595.

Note: See TracTickets for help on using tickets.