Currently the close button is an anchor element. From overlay.tpl.php

<div id="overlay-close-wrapper">
  <a id="overlay-close" href="#" class="overlay-close"><span class="element-invisible"><?php print t('Close overlay'); ?></span></a>
</div>

The anchor element should be updated to a button so that it is made accessible to screen reader users.

Linked from #890288: Improve Overlay accessibility by using modal dialogs.

Files: 
CommentFileSizeAuthor
#12 1964880-12.patch2.66 KBMiroslavBanov
PASSED: [[SimpleTest]]: [MySQL] 59,426 pass(es).
[ View ]
#10 1964880-8-10.inter_.diff850 bytesMiroslavBanov
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1964880-8-10.inter_.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 1964880-10.patch1.27 KBMiroslavBanov
PASSED: [[SimpleTest]]: [MySQL] 59,264 pass(es).
[ View ]
#9 Screen Shot 2013-09-11 at 4.05.24 PM.png185.88 KBmgifford
#8 1964880-8.patch1.28 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,826 pass(es).
[ View ]
#2 overlay-turn_close_link_into_button-2.patch913 bytesfalcon03
PASSED: [[SimpleTest]]: [MySQL] 54,240 pass(es).
[ View ]

Comments

falcon03’s picture

Assigned:Unassigned» falcon03

I can write a patch tomorrow.

falcon03’s picture

Assigned:falcon03» Unassigned
Status:Active» Needs review
StatusFileSize
new913 bytes
PASSED: [[SimpleTest]]: [MySQL] 54,240 pass(es).
[ View ]

Here's the patch. I didn't turned the link in a button element, but I added role="button" to the link. This way nothing should change for sighted users! :D

I've also added aria-controls to the link so that we declare its relation with the overlay content.

Finally, just a question: could we move tabs into the div that the "overlay-content" ID is assigned to?

jessebeach’s picture

Status:Needs review» Reviewed & tested by the community

Looks great, thanks falcon03.

Finally, just a question: could we move tabs into the div that the "overlay-content" ID is assigned to?

Moving the tabs will require some visual styling changes. I noted in a related Overlay restyling issue (#1953374: Implement Seven style guide for core overlay) that we should incorporate this suggested HTML structure change.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 3161890 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

droplet’s picture

Status:Closed (fixed)» Needs work
mgifford’s picture

True, it doesn't use a <button>, but they aren't empty links either.

We could switch it to a button, but it's not an accessibility problem with role="button":

<a id="overlay-close" class="overlay-close" aria-controls="overlay-content" role="button" href="#">
<span class="element-invisible">Close overlay</span>
</a>

@droplet, does that clarify things? Still want it switched to something like:

<button id="overlay-close" href="#" class="overlay-close" aria-controls="overlay-content" type="button"><span class="element-invisible"><?php print t('Close overlay'); ?></span></button>

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,826 pass(es).
[ View ]

Updated with #7 suggestions.

Also added some css styling to remove border added by element.

mgifford’s picture

StatusFileSize
new185.88 KB

Your patch is more consistent with it being a <button>. Similar to #1993894: Contextual quick edit toggle should be a <a role="button"> not a <a> because it tracks on/off state

Applies nicely on SimplyTest.me

Close button for Overlay with patch applied.

MiroslavBanov’s picture

StatusFileSize
new1.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,264 pass(es).
[ View ]
new850 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1964880-8-10.inter_.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed a few issues with #8:
- removed href attribute
- changed closing tag to button as well

mgifford’s picture

Status:Needs review» Needs work

Both of these patches though break accessibility as neither of them have any indication that the button has focus. Neither of them work without using a mouse.

So you can't see that you have focus and can't close the overlay without a mouse.

MiroslavBanov’s picture

StatusFileSize
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,426 pass(es).
[ View ]

Yeah, it didn't even work with a mouse, because the JS specifically checked for Anchor tag. It only appeared to work, because the anchor was cached for me. Here's another attempt.

MiroslavBanov’s picture

Status:Needs work» Needs review
mgifford’s picture

This works! I think it should get some JS review before it gets marked RTBC.

From an accessibility point of view, why did you add:
border: 0;

I could see the very faint outline when navigating to the close button, but am wary about removal of things like that and outlines:
http://www.outlinenone.co

As they are useful for sighted keyboard only users.

MiroslavBanov’s picture

I got the "border: 0" from patch #8. It is because Anchor tags don't have border by default, and have outline on focus. Buttons don't have outline by default and have border by default. The idea is to change the markup, without any visual change. Without border: 0, there is only some displacement/misalignment of the button, without actual visible border.

I also want someone to see the JS change. There is some weirdness where the overlay can be closed with a right-click, but I know that I didn't introduce this, and it works like that with or without the patch. I am not sure if this was intended like that.

mgifford’s picture

Ok, that satisfies me.. Thanks @MiroslavBanov.

So we'll wait for the review to mark it RTBC.

droplet’s picture

Need not to change JS code in my opinion.

and missing role="button" in button element

mgifford’s picture

droplet’s picture

Thanks @mgifford. You correct :) I looked at a wrong example:

#2120011: Remove extra role="button" in button element

MiroslavBanov’s picture

Even the type="button" seems redundant. But I am not sure - there may be issues with old IE if that is removed.

nod_’s picture

It's not redundant, it you omit it, it defaults to type="submit" and you really don't want that. It's in the spec, IE isn't for anything in this one :)

MiroslavBanov’s picture

Actually, there is some confusion on default type, and it is browser (read: IE) dependent. See this:
http://stackoverflow.com/questions/4667979/whats-the-standard-behavior-w...
So the moral is: without any evidence, if you assume there will be a problem with IE, you are probably correct.

nod_’s picture

Status:Needs review» Reviewed & tested by the community

We're in the core queue so a little biksheading won't hurt anyone, moreover we're friday :)

To me IE has the right position (same thing with how it handles padding IE 1 — W3C 0). Since we're talking about having buttons, not submits, looks to me that any problem we'd have would be because of everyone else except IE ; hence IE is not to blame. (I'm not even a fan of IE but you gotta give credit where it's due :)

Anyway, the JS is fine RTBC as per #16

webchick’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

nod_’s picture