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

add accessibleAttribute option to treat as javascript property vs setAttribute #870

Closed
zepumph opened this issue Sep 28, 2018 · 14 comments
Closed
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Sep 28, 2018

From #832 (comment)

Discuss when to have a convenience mixin function call to accessibleAttribute api (like containerAriaRole), and when it should be its own thing completely (inputType).

There are potentially three different ways of setting these on a DOMElement:

element.hidden = true;
element.setAttribute( 'hidden'); // ????????
element.setAttribute( 'hidden', 'true' );

It seems like screen readers and browsers treat these values differently depending on how we set them, see phetsims/sun#397. We should either support both, or decide which one is best and always use that. (I think we need to support both, likely with an option to setAccessibleAttribute).

@zepumph
Copy link
Member Author

zepumph commented Sep 28, 2018

Once we were able to add this option to setAccessibleAttribute (and have peer.setAttributeToElement support it), then mixin methods like setInputValue can directly use the accessible attribute api without failing.

@jessegreenberg
Copy link
Contributor

Here is some documentation about this
https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute

To set the value of a Boolean attribute, such as disabled, you can specify any value. An empty string or the name of the attribute are recommended values.
it looks like the second method in the list would be `b.setAttribute("hidden", "");

However, I recall that we had problems with the screen reader in setting `b.setAttribute( "checked", "checked" ). If this is helpful, we should double check this.

@jessegreenberg
Copy link
Contributor

ll that matters is that if the attribute is present at all, regardless of its actual value, its value is considered to be true. The absence of the attribute means its value is false. By setting the value of the disabled attribute to the empty string (""), we are setting disabled to true, which results in the button being disabled.

This leads me to believe that they should be equivalent, but we have run into problems with this in a few places (at least with AT)

@zepumph
Copy link
Member Author

zepumph commented Oct 26, 2018

This was easy to do so I added it. I think that it is good to have to option as we have random discrepancies between spec and AT implementation. Please review @jessegreenberg.

@jessegreenberg
Copy link
Contributor

Thanks @zepumph, looks good and I verified it is working with the above test.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Nov 13, 2018

Something about this isn't working right for MathML elements, found while looking into #895. Not sure why yet, but attributes like the hidden property are not being removed from MathML elements.

@jessegreenberg
Copy link
Contributor

Here is what I am seeing:
image

@jessegreenberg
Copy link
Contributor

element[ "hidden" ] = true doesn't set the hidden attribute on the element, though element.hidden still returns true.

@jessegreenberg
Copy link
Contributor

Can we replace

element[ attribute ] = attributeValue;

with

element.setAttribute( attribute, "" );

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Nov 14, 2018

Tests (unit, aqua) are passing so going ahead with this. re-closing. I am not sure why the second option in #870 (comment) works but the first doesn't for MathML elements, but that handles the property attributes correctly.

@jessegreenberg
Copy link
Contributor

Oops, getting tired...I forgot to remove the line in 35e36de. When I did that, the checked attribute broke, because we need to remove that attribute.

So with this change, setting an attribute with asProperty will never remove an attribute. And it won't use the property value, should we assert that value isn't used in this case?

@jessegreenberg
Copy link
Contributor

I am understanding now that MathML elements don't support all attributes on HTMLElements, and don't support hidden.

@jessegreenberg
Copy link
Contributor

element.setAttribute( hidden, "" ); makes the hidden attribute show up on MathML elements, but the Math is still visible in the viewport, and https://developer.mozilla.org/en-US/docs/Web/MathML/Attribute doesn't list hidden or any of the attributes of HTML elements. I am going to revert changes because handling attributes for MathML will be a larger issue. To hide MathML maybe we will have to wrap the math with an HTML element that can receive hidden.

@jessegreenberg
Copy link
Contributor

All changes since #870 (comment). Re closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants