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

Remove updateAccessiblePeers #825

Closed
zepumph opened this issue Jul 4, 2018 · 4 comments
Closed

Remove updateAccessiblePeers #825

zepumph opened this issue Jul 4, 2018 · 4 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jul 4, 2018

from #814.

@zepumph
Copy link
Member Author

zepumph commented Jul 4, 2018

@jessegreenberg I moved a lot of code from Accessibility.js over to AccessiblePeer.js. So far this is feeling very straight forward and nice. Please review.

@jessegreenberg
Copy link
Contributor

@zepumph this looks really nice and is in the spirit of MVC. Also, addAssociationImplementationForAttribute is much improved, thanks!

I am seeing checked=false on lots of non input PDOM elements now. Probably because checked is now added with peer.setAttributeToElement() rather than directly with the setter like peer.primarySibling.checked = false. Maybe we need a function in AccessiblePeer just for checked?

Also, in the replacements of updateAccessiblePeers, I am seeing lots of checks like
peer && peer.doStuff().
At those locations is it possible for peer to not be defined?

@zepumph
Copy link
Member Author

zepumph commented Jul 10, 2018

I am seeing checked=false on lots of non input PDOM elements now. Probably because checked is now added with peer.setAttributeToElement() rather than directly with the setter like peer.primarySibling.checked = false. Maybe we need a function in AccessiblePeer just for checked?

I think checked may need more work that just that, for example, see
AquaRadioButton.js (also in RadioButtonGroupMember):

    // a11y - Specify the default value for assistive technology. This attribute is needed in addition to
    // the 'checked' property to mark this element as the default selection since 'checked' may be set before
    // we are finished adding RadioButtons to the containing group, and the browser will remove the boolean
    // 'checked' flag when new buttons are added.
    if ( property.value === value ) {
      this.setAccessibleAttribute( 'checked', 'checked' );
    }

Perhaps we need to rethink how we are handling checked to begin with. @jessegreenberg would it be alright to create another issue for this, and address it after #814? As I think much may change in AccessiblePeer due to that issue, and we may have better insight into how we should proceed.

@zepumph
Copy link
Member Author

zepumph commented Jul 17, 2018

I created a separate issue. Closing

@zepumph zepumph closed this as completed Jul 17, 2018
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