Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#29974 closed defect (bug) (fixed)

Focus handle at wrong place when you click reply

Reported by: nhuja's profile nhuja Owned by: afercia's profile afercia
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords: has-patch has-screenshots
Focuses: accessibility, javascript Cc:

Description

This looks like a WordPress issue. If you are logged in, and in any comment, you click reply, the textarea gets the focus handle. This is fine.

But when you are logged out, and click reply, it again goes to the textarea. I think this should go to Name field. In desktop, this might not be a big issue but in mobile, this is a huge one. You get to the focus where you are prompted with keyboard. Once you fill in the comment and click Done, and Submit (which is usual behaviour), you get to the error page. You will have to come back, scroll to the comment and fill in the name and email again. Since the focus handle (cursor) goes to the Comment field, the Name and Email hides up in the browser view of the mobile and users will not know you have to scroll up and add those fields before you can submit without errors.

Attachments (8)

29974.patch (1.3 KB) - added by afercia 9 years ago.
29974.2.patch (1.6 KB) - added by adamsilverstein 9 years ago.
29974.3.patch (3.1 KB) - added by afercia 9 years ago.
29974.4.patch (2.8 KB) - added by afercia 9 years ago.
29974.5.patch (3.0 KB) - added by afercia 8 years ago.
Adds some JavaScript for the initial focus.
29974.6.patch (3.4 KB) - added by azaozz 8 years ago.
29974.7.patch (3.3 KB) - added by afercia 8 years ago.
29974.8.patch (2.4 KB) - added by afercia 8 years ago.

Download all attachments as: .zip

Change History (69)

#1 @afercia
9 years ago

  • Focuses accessibility javascript added
  • Version changed from trunk to 4.0

There are also some accessibility issues here: when not logged in, when focus gets moved to the textarea screen reader users will not be informed correctly about the form structure.
Previous fields will be completely skipped and users will assume there's only the textarea, unless they tab backwards or move around with arrow keys.
The comment fields are also filterable so there's no guarantee the "Author" field will be the first one. Focus should be moved to the first focusable field, whatever it is.

#2 @afercia
9 years ago

  • Keywords has-patch needs-testing added
  • Version changed from 4.0 to 2.7

To clarify what's currently happening, please refer to the screenshot below.
When you're not logged in:

  • click on Reply
  • focus is moved to the textarea skipping previous form elements

https://cldup.com/tXVZJuMJEl.png

Sooner or later, users have to focus and fill in the other fields too, so why focus the textarea first?
Screen reader users will be "fooled": when focus gets moved they will think there's a good reason for that but they won't realize there are other fields before the textarea until they get the error page or unless they move around the form.

Looks like it always worked this way, since 2.7 and never changed, maybe I'm missing something, please let me know.

Form elements are filterable, it's impossible to predict how many fields will be there and their order, the proposed patch needs to be as generic as possible and just assumes there's at least one form element, then:

  • tries to focus the first not-hidden not-disabled form element, whatever it is
  • uses scrollIntoView()

@afercia
9 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


9 years ago

#4 @joedolson
9 years ago

Looks like a good solution to me. It is very disconcerting for a user to get landed in the middle of a form - especially if you consider cases where the user is usually logged in, but not this time - they'd be expecting to simply be able to submit the form after completing the message.

#5 @davideugenepratt
9 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

I've tested this patch in IE, Firefox, and Chrome and it works as it should.

Edit: Got it. Thanks Jorbin.

Last edited 9 years ago by davideugenepratt (previous) (diff)

#6 @jorbin
9 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Thank you for testing davideugeneprat, however the worksforme status is for the ticket as a whole, not for the patch working.

#7 @nacin
9 years ago

I think I disagree with a change here. If there are accessibility reasons, I'm all ears, but in terms of usability, when you hit "Reply" the first thing you want to do is type your comment, not enter your name.

#8 @joedolson
9 years ago

For accessibility reasons, it's very disorienting to be dropped into a field that's in the middle of a form - you don't know what came before it or after it; you should always be dropped into the first field of the form, whatever that should happen to be.

If WordPress had a smoother comment flow, so that the sequence of Write Comment > Submit Comment > Receive Error requiring you to enter name and email > Hit back button > find & finish form was less messy, this wouldn't be as big of a deal, but otherwise it's very awkward.

Anybody who's commenting will either need to log-in or add their name/email before submitting the form no matter what, so it seems to me that whether they want to write a comment first isn't really the operative usability issue - the usability is improved by adding information in the presented sequence.

The user has no reason to assume that there are any form fields other than the comment form that they need to fill out. Yes, that's standard if this is a WordPress comment form, but it's not the user's job to know what CMS the site they're commenting on is using. And even if it is WordPress, it's still possible that the comments are driven by Disqus, WordPress.com, Facebook, or some other service where they may already be logged in, and not need to add their name and email. They shouldn't have to navigate backwards in the form to discover whether there are additional fields to fill in.

#9 @nhuja
9 years ago

This is what I usually hate about reporting/submitting any patches. The lead developers always think what they did are the best. They don't want to hear any thing else from anyone.

#10 @nhuja
9 years ago

  • Resolution set to invalid
  • Status changed from reopened to closed

#11 @jorbin
9 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

This issue isn't decided, and this is absolutely a valid issue, it shouldn't be closed.

#12 follow-up: @jorbin
9 years ago

I tend to agree with Nacin that the solution here degrades the experience for some users. At the same time, the experience for some users right now is completely subpar and bordering on negative in general.

As this deals with code that is output on the most user facing portion of the site, I think we need to be incredibly cautious about changes that we make. This prevents a change to the HTML order while keeping the existing visual order.

Taking these into account, I wonder if perhaps something that would improve the experience for screen reader users while not degrading the experience for others would be to add some screen-reader only text to .form-allowed-tags which is the aria-describedby for the comment field that let users know there are X fields in total and this text could be added only when users clicked the reply link.

#13 follow-up: @joedolson
9 years ago

That sounds over-engineered to me; but I'm uncertain about how the change (focusing on the first field in the form) would degrade the experience for some users? What is the deterioration? I may just not be seeing it.

I don't think there's any proposal to change the order of HTML fields; just to modify the target of #reply focusing based on which fields exist in the form.

#14 in reply to: ↑ 13 @nacin
9 years ago

Replying to joedolson:

That sounds over-engineered to me; but I'm uncertain about how the change (focusing on the first field in the form) would degrade the experience for some users? What is the deterioration? I may just not be seeing it.

Just think about how much muscle memory would be broken with a change like this. I don't want to make any change here lightly.

#15 @TimothyBlynJacobs
9 years ago

This isn't only an accessibility issue though. As originally mentioned, when on mobile chances are the user won't even see the prepended fields.

#16 @joedolson
9 years ago

I'm not sure I buy the muscle memory argument. That will effect only a small minority of users who are heavy commenters; and only if they aren't logged into the site in question, which means it's not likely to effect site owners or any other logged-in user. Those users can learn new habits pretty easily, I'd guess. For the massive majority of commenters, however, "muscle memory" is fairly nominal, since they are likely not to be particularly familiar with what any given sites comment fields are doing.

#17 in reply to: ↑ 12 @afercia
9 years ago

Replying to jorbin:

... add some screen-reader only text to .form-allowed-tags which is the aria-describedby for the comment field that let users know there are X fields in total ...

Hi Jorbin, found there's already a proposal to remove the allowed tags by default, see #30157 and looks like there's some consensus about that, see also the comments on the original post by Brian Krogsgard.

Moreover, as pointed out by kpdesign, comment_notes_after is often overridden by themes/plugins to provide a completely different text (e.g. comment policy, whatever), so there's no guarantee it will convey a real description or effective instructions about the form.

Generally speaking, I would recommend to ignore for a while our personal opinions, muscle memory, habits, and actually test this using only the keyboard and also testing with a screen reader. In both cases, you will probably notice the current tab order is not ideal because the logical structure of the form is not ideal.

Replying to nacin:

in terms of usability, when you hit "Reply" the first thing you want to do is type your comment, not enter your name.

Hi Nacin, got your point. IMHO, If you want a specific form field to be filled first, then it should be the first field in the form in the first place. Moving focus is just a workaround and in most cases it will introduce usability bugs rather than solving them.

Consider for example Jetpack comments module, which already makes the textarea the first field in the form, and it does that for a good reason. Additional fields should come after the textarea, if and when they're needed.

I really don't know if there are strong arguments against changing the fields order, but that seems to me the best solution. Would really appreciate any thoughts and feedback.

#18 @adamsilverstein
9 years ago

29974.2.patch moves the comment field to the top of the form for experimentation :)

http://f.cl.ly/items/2M172v40402t1Q2O2I40/test__sadasd_2014-12-29_12-02-39.jpg

#19 @helen
9 years ago

I agree that having to reverse-tab to required fields is very counter-intuitive, especially when our comment erroring flow is terrible (#4332). The idea of moving the field up intrigues me - muscle and visual memory is indeed a powerful thing, but when looking at other commenting platforms (included in the WP arena), having the comment field first is not atypical by any means. The only thing that Adam's experiment exposes to me is that the allowed tags blob isn't really in the right spot - perhaps it should be swapped with the email disclaimer/required fields text.

#20 follow-up: @afercia
9 years ago

Thanks very much @adamsilverstein, @helen, glad to see some consensus here :)
Noticed there are some hooks to take into account, some of them are available conditionally depending on discussion settings/logged in status. There are also some conditional messages. It looks like in the current patch the comment textarea is a bit misplaced, in the refreshed patch I'm trying to tweak it a bit. Not so happy to introduce a new if but couldn't think of a better way.

Also, I would propose to:

  1. remove a useless title attribute on the "Log out?" link
  2. use an aria-label attribute on the "Logged in as {display name}", see what was already done for the "Reply to" link in r29822

About 2. that link points to your profile but the link text is your display name, this could be confusing when read out of context. To be honest, I think it's not so clear also for sighted users :)
See screenshot below: it's just your display name, doesn't say anything about the link target.

https://cldup.com/-rSHevBw3A.png

I understand that string now is not so nice for translation purposes, but it's not the only problematic string there. Open to suggestions and any thoughts welcome.

@afercia
9 years ago

#21 @adamsilverstein
9 years ago

  • Keywords dev-feedback added; needs-testing removed

Nice work @afercia, thanks. Verified focus in correct location when clicking reply.

Screenshot:

http://f.cl.ly/items/0A2r3i0Z1S3l2C1M0m3Z/Post_21__sadasd_2015-01-23_16-17-29.jpg

#22 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov
9 years ago

Replying to afercia:

Also, I would propose to:

  1. remove a useless title attribute on the "Log out?" link
  2. use an aria-label attribute on the "Logged in as {display name}", see what was already done for the "Reply to" link in r29822

With the aria-label attribute, does "Logged in as" still need to be a part of the link?

#23 in reply to: ↑ 22 @afercia
9 years ago

Replying to SergeyBiryukov:

With the aria-label attribute, does "Logged in as" still need to be a part of the link?

It's a bit tricky, basically screen readers should report as link text just the aria-label and ignore the actual text. Tested in Firefox + NVDA:

https://cldup.com/hIXwnKF9QP.png

That's also how r29822 is intended to work.

Moving out "Logged in as" would make screen readers read out:

Logged in as
link
Logged in as Andrea. Edit your profile.

At this point we should hide "Logged in as" from screen readers, something like:

<span aria-hidden="true">Logged in as</span>
<a href="%1$s" aria-label="Logged in as %2$s. Edit your profile.">%2$s.</a> <a href="%3$s">Log out?</a>

And that string would be even more problematic. I agree it's counterintuitive but that's because I didn't want to make any visual change. It would be nice to simplify all this, if there's consensus for a small visual change:

Logged in as Andrea. <a ...>Edit your profile.</a> <a ...>Log out?</a>

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


9 years ago

#25 @afercia
9 years ago

  • Keywords 4.3-early added

Would like to propose this for 4.3-early consideration

#26 @obenland
9 years ago

  • Owner set to jorbin
  • Status changed from reopened to assigned

#27 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Awaiting Review to 4.3

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


9 years ago

@afercia
9 years ago

#29 @afercia
9 years ago

  • Keywords changed from has-patch, dev-feedback to has-patch dev-feedback

Refreshed patch.

#30 follow-up: @GrahamArmfield
9 years ago

The patch works fine for me and I can confirm Afercia's findings with a screen reader (I used NVDA and Firefox). For me it makes sense to have the comments box first.

One small problem I've noticed: Above the input fields there is text to tell people that all mandatory fields are indicated with an asterisk. However, the comment box carries the required and aria-required attributes, but there is no visual indication the the field is required.

#31 in reply to: ↑ 30 ; follow-up: @afercia
9 years ago

Replying to GrahamArmfield:

the comment box carries the required and aria-required attributes, but there is no visual indication the the field is required.

I think this is because when users are logged in, the comment box is the only form field displayed by default and the message 'Required fields are marked *' doesn't show up either. Open to suggestions :)

#32 follow-up: @jorbin
9 years ago

  • Keywords early added
  • Milestone changed from 4.3 to Future Release

Sorry, i missed this being assigned to me.

I worry that changing the order of do_action( 'comment_form_after_fields' ); and echo apply_filters( 'comment_form_field_comment', $args['comment_field'] ); is going to be a breaking change. I am going to move this out of 4.3 since it is already beta and this type of change really needs as much testing as possible to ensure things don't break.

#33 in reply to: ↑ 32 @afercia
9 years ago

Replying to jorbin:

I am going to move this out of 4.3 since it is already beta and this type of change really needs as much testing as possible to ensure things don't break.

@jorbin do you think we can give this a try since we're now at the beginning of a new release cycle?

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


9 years ago

This ticket was mentioned in Slack in #design by afercia. View the logs.


9 years ago

#36 @helen
9 years ago

  • Milestone changed from Future Release to 4.4

Let's try this now and not later. :)

#37 @wonderboymusic
9 years ago

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

In 34525:

Comments: In comment_form(), move the comment textarea to the top for logged-out users when replying, improves keyboard/focus navigation.

Rehabilitate errant tabbing and extra unnecessary <?php ?> toggling.

Props afercia, adamsilverstein.
Fixes #29974.

#38 @ryan
9 years ago

  • Keywords has-screenshots needs-screenshots added

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


9 years ago

#40 follow-up: @mindctrl
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This will break any CSS that targets elements based on position, for example :first-of-type. The same goes for jQuery's :first-of-type selector. Why can't the order remain the same and just give the name field focus?

#41 in reply to: ↑ 40 @helen
9 years ago

  • Keywords dev-feedback has-screenshots needs-screenshots removed

Replying to mindctrl:

This will break any CSS that targets elements based on position, for example :first-of-type. The same goes for jQuery's :first-of-type selector.

I'm sympathetic to this, but why would that be done blindly? That's a very fragile thing to be doing when it's not your mark up. If we're not providing specific enough mark up for styling, we should take a look at that for sure.

Why can't the order remain the same and just give the name field focus?

It's different when you're logged in and when you're not. We could make it different for each, but there's a lot to be said for consistency and putting what's typically the most important entry field first.

#42 follow-up: @mindctrl
9 years ago

I'm sympathetic to this, but why would that be done blindly? That's a very fragile thing to be doing when it's not your mark up. If we're not providing specific enough mark up for styling, we should take a look at that for sure.

Understandable, but the field order has been the same for a long time, I think? It's safe to assume people do all sorts of crazy things. Highlighting a particular field, running a script to detect valid URLs, whatever. I agree that it's a fragile thing to do. Just want to be sure this is the right thing.

It's different when you're logged in and when you're not. We could make it different for each, but there's a lot to be said for consistency and putting what's typically the most important entry field first.

I can see that too. I think it's reasonable to expect that if I'm logged into a site, I won't have to enter my name, email, and website. Many/most sites have different experiences based on logged in status. If consistency is the concern, we're not being consistent with years of history, or consistent with the placement of the textarea in relation to the submit button. If I'm logged in, I get a textarea > submit. If I'm logged out, I get textarea > input > input > input > submit.

The only consistent thing in my mind that is the least likely to break things is to keep the order as it's been for years and just change the focus. Unless I'm missing something, and I very well could be since comments are my least favorite thing to deal with. :)

#43 in reply to: ↑ 42 @helen
9 years ago

Replying to mindctrl:

If consistency is the concern, we're not being consistent with years of history, or consistent with the placement of the textarea in relation to the submit button.

Yep, consistency has many different forms. I definitely have some concerns about this interfering with people's habits, but I felt that overall the change would be a good one. I did take a look at other comment forms around the web, which go either way but many put comment field first, and Jetpack/.com comment forms put the comment field first, FWIW.

#44 in reply to: ↑ 31 @GaryJ
9 years ago

Replying to afercia:

Replying to GrahamArmfield:

the comment box carries the required and aria-required attributes, but there is no visual indication the the field is required.

I think this is because when users are logged in, the comment box is the only form field displayed by default and the message 'Required fields are marked *' doesn't show up either. Open to suggestions :)

The asterisk for the Comment field is also missing from the logged out view too (which I think is what Graham was referring to).

#45 @vinorodrigues
9 years ago

Thanks for adding the title_reply_befor / -after in 4.4 ... this has always been something that I've overwrote in my themes (the h3 tag hard coded into the otherwise dynamic function). Something like the "submit_field" arg would have been easier tho...

But... the "cancel reply" link in the title still does not make sense to me. It would be better elsewhere... like as a button/link next to the submit button. Please consider moving this or making it a dynamic positioning thing. (Just my 2¢...)

#46 @ryan
9 years ago

  • Keywords has-screenshots needs-screenshots added

There are screenshots on this post. I'll handle mobile screenshots.

https://make.wordpress.org/core/2015/09/25/changes-to-fields-output-by-comment_form-in-wordpress-4-4/

#47 follow-up: @greenshady
9 years ago

This is a continuation of the discussion on the Make post: https://make.wordpress.org/core/2015/09/25/changes-to-fields-output-by-comment_form-in-wordpress-4-4/comment-page-1/#comment-27867

We're talking about making a major front end visual change to millions of sites (a welcome change for those of us who care about accessibility). We're talking 10 years of history here. There needs to be the ability for a plugin/theme author to hook in and revert the comment field position change.

I could be wrong in assuming that the support forums will be filled with angry users wondering why their comment forms changed suddenly. I could be wrong in assuming that my theme forums will be filled with "WTF did you do to my site" topics. I've been around too long to be that optimistic though. We need to learn from the mistakes of the past when such changes happen. I think this is clearly one of those times where we need to take a step back and realize that this is a breaking change for many users, even though it fixes a longstanding accessibility bug.

What I'd like to see is a parameter for comment_form() called comment_field_position (or something like that), which can default to the new position. That way, at least someone can create a plugin that will revert it to the old position (I'll even volunteer to build the plugin). That will mitigate some of the support tickets and keep us from having to copy/paste the standard "this is a bug fix" reply.

I'd also still like for the comment reply JS to focus on the first field in the form, regardless of what that field is. There are legitimate reasons to have fields before the comment field (a star rating field immediately comes to mind). Sure, a plugin dev could roll their own JS, but core could just as easily simply focus on the first focus-able field.

#48 in reply to: ↑ 47 ; follow-up: @helen
8 years ago

Replying to greenshady:

We need to learn from the mistakes of the past when such changes happen. I think this is clearly one of those times where we need to take a step back and realize that this is a breaking change for many users, even though it fixes a longstanding accessibility bug.

There's a Make/Core post because we know it's a breaking change and needs feedback. I think what we're seeing today is that there are more committers and a number are more aggressive when it comes to what lands in trunk - not waiting for something to be perfect before trying it out. Realistically, the number of people who test things in patch form is tiny. Reverts are easy, and more feedback leads to better improvements. I think generally it also helps tickets keep moving instead of stagnating.

What I'd like to see is a parameter for comment_form() called comment_field_position (or something like that), which can default to the new position.

That's the kind of thing that ends up as technical debt in the future, but it does seem like it will be necessary to either do something along those lines and/or provide very clear documentation on how to change it back. If what's there doesn't allow changing back via hooks, that should be looked at as well.

I'd also still like for the comment reply JS to focus on the first field in the form, regardless of what that field is.

I didn't realize this didn't also go in :) We should do that, yes.

#49 @johnbillion
8 years ago

#24064 was marked as a duplicate.

#50 in reply to: ↑ 48 @DrewAPicture
8 years ago

  • Keywords early needs-screenshots removed
  • Owner changed from jorbin to afercia
  • Status changed from reopened to assigned

@afercia: Could you take a look at the last part of comment:48 so we can finish this up?

Replying to helen:

Replying to greenshady:

I'd also still like for the comment reply JS to focus on the first field in the form, regardless of what that field is.

I didn't realize this didn't also go in :) We should do that, yes.

@afercia
8 years ago

Adds some JavaScript for the initial focus.

#51 follow-up: @afercia
8 years ago

Patch 29974.5.patch adds some JavaScript to set focus on the first form focusable element. Also, some cleaning and coding standards.

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


8 years ago

@azaozz
8 years ago

#53 in reply to: ↑ 51 @azaozz
8 years ago

Replying to afercia:

29974.5.patch looks good. In 29974.6.patch added checks for readonly and hidden from CSS and removed respond.scrollIntoView(); as it can scroll the focused element off the screen in some cases (when the textarea is quite tall).

@afercia
8 years ago

#54 follow-up: @afercia
8 years ago

Discussed a bit with @azaozz and agreed readonly form elements should be allowed to get focus because they're focusable :)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#56 @afercia
8 years ago

In 35593:

Comments: in comment_form() when replying to a comment ensure to set focus on the first focusable form element, regardless of what that form element is.

Props azaozz.
See #29974.

#57 in reply to: ↑ 54 ; follow-up: @azaozz
8 years ago

Replying to afercia:

Uh, the props should have been afercia, azaozz. I just tweaked your patch a bit :)

#58 in reply to: ↑ 57 @afercia
8 years ago

Replying to azaozz:

Uh, the props should have been afercia, azaozz. I just tweaked your patch a bit :)

Next time :)
Looking back at this, thinking the visibility check fails when an ancestor has visibility: hidden and a closer ancestor (or the element itself) has visibility: visible. Working on a new patch.

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


8 years ago

@afercia
8 years ago

#60 @afercia
8 years ago

Refreshed patch, trying to simplify a bit and improve checking for visibility that fails in some edge cases, see for example: http://s.codepen.io/afercia/debug/YyRpzw

  • extends support to IE 8
  • no need for a while loop to recursively check the parent nodes
  • improves checking for elements hidden with CSS:
    • for visibility: getComputedStyle() / currentStyle already return the actual computed value, taking into account the ancestors visibility (doesn't work in IE 7-)
    • for display none: do the same thing jQuery does, checking for offsetWidth and offsetHeight

Would greatly appreciate feedback and JavaScript review :)

#61 @wonderboymusic
8 years ago

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

In 35675:

Comments: after [35593], extend support to IE8 and improve checking for elements hidden with CSS

Props afercia.
Fixes #29974.

Note: See TracTickets for help on using tickets.