Currently when #title_display is set to 'invisible' the label generated for the form element comes after the form element in markup. This is confusing for screen-reader users (by which I mean myself). It is also not an intuitive way for a form to be rendered.

I'm not sure why we decided this was a good idea, but it's not, so let's reverse the order so that the invisible label is output prior to the form element.

CommentFileSizeAuthor
#6 917220-2.patch2.79 KBmgifford
#1 917220-1.patch787 bytesEverett Zufelt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

FileSize
787 bytes

Patch.

Let me know if the indentation is off, it seemed odd to me in the original

Everett Zufelt’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 917220-1.patch, failed testing.

Everett Zufelt’s picture

If someone would be able to rewrite the test case for this I would be greatly appreciative. Looks like we actually have a test looking for a label after the element, and it needs to look before.

mgifford’s picture

Status: Needs work » Needs review

I think this addresses why the patch failed.

I don't know how critical this is, but it's probably more standard to have it above.

mgifford’s picture

FileSize
2.79 KB

ack, forgot to attach the file. Sorry.

Status: Needs review » Needs work

The last submitted patch, 917220-2.patch, failed testing.

mgifford’s picture

Everett Zufelt’s picture

Status: Needs work » Needs review

#6: 917220-2.patch queued for re-testing.

bowersox’s picture

+1 for this change. @Everett, I agree that this order for the label is much more usable. I reviewed the patch and it looks good to me.

Everett Zufelt’s picture

Thankfully it passed all the tests this time. @brandonojc can you please RTBC?

bowersox’s picture

Status: Needs review » Reviewed & tested by the community

Okay, I tested this and it works well. I can see the different on a page like /admin/people/permissions where it is much more helpful to have the labels precede the checkboxes. This patch appears to work correctly. Nice one!

sun’s picture

#6: 917220-2.patch queued for re-testing.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Everett Zufelt’s picture

Version: 8.x-dev » 7.x-dev

Not disagreeing. But, if you bump one of my issues provide clear documentation to support your claim.

mgifford’s picture

This patch also seems to be pretty low-risk. I don't see how when a patch improves a user experience & has almost no risk of impacting anyone else why it shouldn't come into core. I can see this being one that shouldn't be brought in with a maintenance/security release as that could break existing tests. However, at this stage, it already passes the bots & improves the experience. It's also been marked RTBC already.

Everett Zufelt’s picture

Issue tags: +Quick fix

Tagging

webchick’s picture

Are there visual changes associated with this patch? If so, could we have a before/after screenshot? Or is this purely dealing with where on the page the invisible text shows up?

Everett Zufelt’s picture

@webchick

Absolutely no visual changes, just changes markup.

When #title_display is set to invisible the markup for the invisible label is currently being generated after the form field, this patch moves the markup for the invisible label to before the form field. In either case the label is invisible, hidden with .element-invisible.

Thanks.

mgifford’s picture

There are no visual changes. I can provide a screenshot if you like, but it is just dealing with the invisible text.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok cool, thanks!

Committed to HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Accessibility

Automatically closed -- issue fixed for 2 weeks with no activity.