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

Fixing #57 Moving focus to next item (in paper-menu) does not skip disabled items. #58

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

giltza
Copy link
Contributor

@giltza giltza commented Apr 7, 2016

Fixing #57
We are looking at the 'disabled' html dom attribute rather then the iron-control-state 'disabled' property for more generic support.

@tjsavage
Copy link
Contributor

tjsavage commented Apr 8, 2016

One thing we should check is a11y - it might actually be the right behavior to tab through disabled state items, so that the screen reader can read the item. @bicknellr and @notwaldorf what do you think?

@giltza
Copy link
Contributor Author

giltza commented Apr 14, 2016

@tjsavage , @notwaldorf , @bicknellr
Let me know what to do.
I personally think that traversing through disabled items has no purpose and the standard is to skip them (Check out menus in your OS).
But I have not problem adding a property configuration that will be default to the current state.

@bicknellr bicknellr self-assigned this Apr 15, 2016
@bicknellr
Copy link
Contributor

I think it's pretty far-fetched to say that people would be using an attribute called disabled on a item of a selectable set for something other than making that item unselectable, but it's possible.. Maybe it could be used to disable something internal to that item? @notwaldorf, care to perform the ritual weighing of the change?

@notwaldorf
Copy link
Contributor

Pretty much every native element that I can think of is skipped when disabled: http://jsbin.com/leyoja/edit?html,output, so I'm fairly sure that the platform intention is to skip disabled inputs.

So then I nerded out and dug out the spec and it says:

An [...] element that is disabled must prevent any click events that are queued on the user interaction task source from being dispatched on the element.

which also suggests that disabled items should not be interacted with (and tabbing sounds like an interaction, I think)

@@ -94,6 +105,74 @@
});
});

test('focusing on next item skips disabled items', function(done) {
var menu = fixture('disabled');
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debugger!

@bicknellr
Copy link
Contributor

@giltza, sounds like this is ok without a flag. There's a debugger in the tests and I think it would be nice to have a test that checks that these won't loop forever / otherwise break if all the items are disabled. (They look fine at first glance though.) Otherwise, LGTM.

Also, @notwaldorf: 🍰 is as close as I could get; no :brownie:s were available.

…oes not skip disabled items.

We are looking at the 'disabled' html dom attribute rather then the iron-control-state 'disabled' property for more generic support.
@giltza
Copy link
Contributor Author

giltza commented Apr 17, 2016

@tjsavage , @notwaldorf , @bicknellr
a. Removed debugger; (sorry about that)
b. Made sure that in case of all disabled item menus, no item gets focused on menu focus.
c. Added additional tests for empty menus and all disabled item menus.

@bicknellr
Copy link
Contributor

Great, thanks!

@bicknellr bicknellr merged commit 57ebc50 into PolymerElements:master Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants