WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 13 months ago

#15080 closed enhancement (fixed)

Comment Form Should use HTML5 input types for better accessibility

Reported by: jorbin Owned by: 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 4 years ago.
15080.patch (2.1 KB) - added by jorbin 19 months ago.
15080.updated.patch (2.1 KB) - added by lessbloat 18 months ago.
15080.args.diff (4.1 KB) - added by jorbin 14 months ago.
15080.args.2.diff (4.1 KB) - added by jorbin 14 months ago.
15080.3.diff (3.5 KB) - added by georgestephanis 14 months ago.
15080.4.diff (2.8 KB) - added by obenland 14 months ago.
Simpler approach without input builder
15080.5.diff (2.9 KB) - added by obenland 14 months ago.
Simpler approach without input builder, but with empty pattern attributes
15080.6.diff (2.9 KB) - added by georgestephanis 13 months ago.
15080.7.diff (5.2 KB) - added by sscovil 13 months ago.
Wrapped comment form help text in label tags for accessibility.
15080.8.diff (5.1 KB) - added by sscovil 13 months ago.
Removed 'for' attributes from new labels added for accessibility in 15080.7.

Download all attachments as: .zip

Change History (67)

jorbin4 years ago

comment:1 nacin4 years ago

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

comment:2 demetris4 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.

comment:3 demetris4 years ago

Another problematic aspect I forgot to mention:

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

comment:4 follow-up: jorbin4 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.

comment:5 in reply to: ↑ 4 demetris4 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.

comment:6 westi3 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

comment:7 follow-up: alex-ye3 years ago

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

comment:8 in reply to: ↑ 7 GaryJ3 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".

comment:9 Elpie3 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 3 years ago by Elpie (previous) (diff)

comment:10 markjaquith3 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.

comment:11 nacin3 years ago

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

Let's do this.

comment:12 husobj22 months ago

  • Cc ben@… added

comment:13 mdawaffe19 months ago

Patch still applies cleanly with offset.

comment:14 nacin19 months 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.

jorbin19 months ago

comment:15 jorbin19 months ago

Absolutely worth a chance.

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

comment:16 bpetty19 months 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.

comment:17 bpetty19 months 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.

comment:18 helenyhou18 months ago

  • Keywords needs-refresh added; 3.2-early removed

lessbloat18 months ago

comment:19 lessbloat18 months 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.

comment:20 helenyhou18 months ago

  • Keywords commit needs-refresh removed

comment:21 DrewAPicture18 months 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.

comment:22 helenyhou18 months ago

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

comment:23 sabreuse18 months ago

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

comment:24 georgestephanis18 months 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.

comment:25 georgestephanis18 months ago

Revised patch attached after talking to jorbin

comment:26 jorbin18 months 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]

comment:27 nacin18 months 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.

comment:28 retlehs15 months ago

  • Cc retlehs added

comment:29 jorbin14 months 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.

jorbin14 months ago

jorbin14 months ago

comment:30 lancewillett14 months 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.

comment:31 SergeyBiryukov14 months ago

#23614 was marked as a duplicate.

comment:32 azizur14 months ago

  • Cc azizur added

georgestephanis14 months ago

comment:33 follow-up: georgestephanis14 months 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.

obenland14 months ago

Simpler approach without input builder

comment:34 follow-up: obenland14 months 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 14 months ago by lancewillett (previous) (diff)

comment:35 lancewillett14 months ago

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

comment:36 in reply to: ↑ 34 jorbin14 months 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.

comment:37 esmi14 months 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.

obenland14 months ago

Simpler approach without input builder, but with empty pattern attributes

comment:38 obenland14 months 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?

comment:39 sscovil14 months ago

  • Cc sscovil@… added

comment:40 esmi14 months 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.

comment:41 in reply to: ↑ 33 SergeyBiryukov14 months 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

comment:42 SergeyBiryukov13 months ago

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

georgestephanis13 months ago

comment:43 georgestephanis13 months ago

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

comment:44 SergeyBiryukov13 months ago

  • Keywords commit added

comment:45 sscovil13 months ago

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

sscovil13 months ago

Wrapped comment form help text in label tags for accessibility.

comment:46 sscovil13 months ago

The old comment form output some 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 latest patch resolves this issue by wrapping form help text in label tags, as seen here: http://pastebin.com/yzpBBTD4

Version 0, edited 13 months ago by sscovil (next)

comment:47 obenland13 months 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.

comment:48 sscovil13 months 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 13 months ago by sscovil (previous) (diff)

sscovil13 months ago

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

comment:49 sscovil13 months 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 13 months ago by sscovil (previous) (diff)

comment:50 georgestephanis13 months 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

comment:51 sscovil13 months ago

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

comment:52 follow-up: jorbin13 months 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

comment:53 esmi13 months 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 "You 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.

Last edited 13 months ago by esmi (previous) (diff)

comment:54 sscovil13 months 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.

comment:55 in reply to: ↑ 52 lancewillett13 months 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.

comment:56 markjaquith13 months 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

Note: See TracTickets for help on using tickets.