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

Disabled checkboxes should not be tab navigable #517

Closed
zepumph opened this issue Jul 8, 2019 · 8 comments
Closed

Disabled checkboxes should not be tab navigable #517

zepumph opened this issue Jul 8, 2019 · 8 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jul 8, 2019

From phetsims/inverse-square-law-common#66 (comment)

It seems like though the checkbox is properly disabled and can't be toggled, when a checkbox is not enabled, you can still tab navigate to it, and it has focus highlight.

See here: http://localhost/sun/sun_en.html?brand=phet&ea&a11y&screens=2&component=Checkbox

image

@jessegreenberg in html, how do you think the best way to accomplish this is? From a small bit of poking, it seems like toggling the "disabled" attribute based on the state of the enabledProperty will probably work out nicely.

Also note that this is not solely a checkbox problem, but any PhET component that runs on enabledProperty. Likely this is code that we should not spend too much time implementing until it can be added in a single place: to the solution in #257 rather than scattered all over common code.

@zepumph
Copy link
Member Author

zepumph commented Jul 8, 2019

Here is a patch that I believe does what we are interested in accomplishing. I didn't commit because it wasn't totally trivial, and I would rather just use it as a discussion point to try to boost the priority of #257

Index: js/Checkbox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Checkbox.js	(revision e0a45b5ebad14e37eec8c53fbbc306a7e8933a15)
+++ js/Checkbox.js	(date 1562607825480)
@@ -160,7 +160,13 @@
     } );
 
     var enabledListener = function( enabled ) {
-      !enabled && self.interruptSubtreeInput(); // interrupt interaction
+      if ( enabled ) {
+        self.hasAccessibleAttribute( 'disabled' ) && self.removeAccessibleAttribute( 'disabled' );
+      }
+      else {
+        self.interruptSubtreeInput(); // interrupt interaction
+        self.setAccessibleAttribute( 'disabled', true );
+      }
       self.pickable = enabled;
       self.opacity = enabled ? 1 : options.disabledOpacity;
     };

@zepumph
Copy link
Member Author

zepumph commented Jul 8, 2019

See phetsims/gravity-force-lab#110 for the GFLB main issue.

@jessegreenberg
Copy link
Contributor

Proposed change looks very good, however I recall that last we discussed this we decided that disabled components should be focusable but not interactive. I don't remember why we decided that because that is different from the native disabled attribute, but I am going to try to find any discussion notes about it.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jul 8, 2019

Found it! #317 (specifically #317 (comment))

I think we replace disabled in the proposed change with aria-disabled and then make sure ourselves that the checkbox cannot be checked.

@zepumph
Copy link
Member Author

zepumph commented Jul 8, 2019

Gotcha! Thanks for looking into this.

@zepumph
Copy link
Member Author

zepumph commented Jul 8, 2019

We don't need this done until GFL is published, so I think for now this should be put on hold until #257 is done. We can take it off of hold before then if we are getting close to publishing that sim.

@zepumph
Copy link
Member Author

zepumph commented Jul 30, 2019

Actually I think we should mark this off hold. Especially with #519, we should just solve these problems now, together as a batch.

@zepumph
Copy link
Member Author

zepumph commented Jul 30, 2019

Over in #519 we found a solution that can keep diabled checkboxes in the tab order. Based on previous discussion referenced in #517 (comment) it seems like that is the way to go. Closing

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