Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispatch events caught on non-primary siblings #949

Closed
jessegreenberg opened this issue Feb 27, 2019 · 10 comments
Closed

Dispatch events caught on non-primary siblings #949

jessegreenberg opened this issue Feb 27, 2019 · 10 comments

Comments

@jessegreenberg
Copy link
Contributor

Related to #888 (centralizing a11y input), identified in #852 (support a11y on mobile devices) and part of phetsims/a11y-research#133.

We have been assuming that a11y events will only be received by the AccessiblePeer's primary sibling. This is true usually, but when a screen reader is enabled the DOM event can be sent to labels and descriptions if the virtual cursor is on the label/description. This is particularly true for the click event. When reading the label of an element associated with the for attribute, pressing enter will send the click event to the label.

In this case getTrailID is unable to parse the trail ID from the label/description because these elements don't have a data-trail-id attribute. The most direct fix is to add this attribute to all AccessiblePeer siblings. @zepumph does this seem reasonable to you?

@jessegreenberg
Copy link
Contributor Author

This is particularly true for the click event. When reading the label of an element associated with the for attribute, pressing enter will send the click event to the label.

So far, this is the only case I have encountered. On this element NVDA says "clickable" before reading the label. So I am going to proceed with adding data-trail-id to labels, but not description and parent container siblings.

@jessegreenberg
Copy link
Contributor Author

I reverted the change because the commit accidentally added some things that I didn't notice for 2077a2b. Sorry. Ill try again...

@jessegreenberg
Copy link
Contributor Author

OK, that commit looks better.

I tested with NVDA and the click event is no longer throwing errors because the data-trail-id couldn't be found. I am going to leave open until phetsims/a11y-research#133 is resolved since this was wrapped up in that.

@jessegreenberg
Copy link
Contributor Author

I think for completeness all attributes should have data-trail-id so that the event from any element that the device happens to send an event to (container, label, description, whatever). So the addition of this attribute should likely be moved to AccessibilityUtil.createElement.

@jessegreenberg
Copy link
Contributor Author

This was done, I tested with NVDA and check boxes can be clicked by pressing enter/spacebar when the virtual cursor is on the label of the elements even before the checkbox has focus. Closing.

@zepumph
Copy link
Member

zepumph commented Mar 28, 2019

Hey! I have a few questions about this one. To me this feels like a pretty large change, but perhaps that stems from my lesser understanding of AT.

After this change, is it possible that many more events could "sneak" into scenery. Is this the sort of thing we want control of, instead of just turning it all on/off.

Here are a few questions.


primarySibling: div with application role
descriptionSibling: p tag

So now if the virtual cursor is on the description tag, can I completely control the application. This seems weird? Is it?


if you call a node "focusable," it only adds tab index to the primarySibling. Is that still right? Or do other siblings now get tab index?


primarySibling: something you give keydown events to
parentContainer: div

Since keydown bubbles, do we get two keydown events on that Node? One on the primary and the other as it bubbles through the parent?

@jessegreenberg
Copy link
Contributor Author

Sorry to close too soon @zepumph, I should have requested review.

TLDR: The title of this issue is bad, we aren't doing anything special to dispatch events from non-primary siblings. We are just adding the data-trail-id attribute to all siblings so that if the AT sends a DOM event to something other than the primary sibling we can get the Trail to dispatch a Scenery event to the right Node.

I discovered that NVDA can click checkboxes if the virtual cursor is just on the label. The target of the click event in this case is the label element instead of the input element. I suspect this flies for most apps because the browser knows how to associate the label and the input by the for attribute. By giving the label a data-trail-id, the event can then be handled by scenery just like our events on primary siblings. I thought it best to do the same for all sibling elements in case there are other cases like the checkbox label.

After this change, is it possible that many more events could "sneak" into scenery.

Do you mean duplicate events since multiple siblings can receive them? I wouldn't think so, this should be handled by the AT. The AT does'nt send a click event to both the label and the input.

primarySibling: div with application role
descriptionSibling: p tag

So now if the virtual cursor is on the description tag, can I completely control the application. This seems weird? Is it?

I wouldn't expect that, for

<div role="application">Draggable thing</div>
<p>This is how you drag the thing</p>

The virtual cursor is outside the application element so no key events should be sent to the browser.

if you call a node "focusable," it only adds tab index to the primarySibling. Is that still right? Or do other siblings now get tab index?

The primary sibling is still the only one that is "focusable" and the only one that receives a tabindex.

primarySibling: something you give keydown events to
parentContainer: div

Same answer as the application question. The AT should send a keydown event to one element or the other. If it was sending the event to the parentContainer, we would have hit this some time ago.

        assert && assert( j !== children.length - 1, 'unable to find node from unique Trail id' );

@zepumph
Copy link
Member

zepumph commented Apr 22, 2019

This looks great

There is a bit of duplication with all 4 of the DOM elements in AccessiblePeer.

image

perhaps a wrapper like AccessiblePeer.createElement which calls over to AccessibilityUtil.createElement could be nice. If it isn't obvious or feels contrived, feel free to close.

@jessegreenberg
Copy link
Contributor Author

That was a good idea, I made a createElement in AccessiblePeer that reduced the duplication for at least one option and the string concatenation for ids. @zepumph back to you for review.

@zepumph
Copy link
Member

zepumph commented Apr 24, 2019

Looks great thanks

@zepumph zepumph closed this as completed Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants