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

Handle tabindex attribute correctly #83

Merged
merged 2 commits into from
Mar 7, 2018

Conversation

rslawik
Copy link
Contributor

@rslawik rslawik commented Feb 20, 2018

Fixes #76. It is the second attempt after #78 and #79.

We need to remove tabindex attribute if it was not set before disabled was activated. Otherwise (leaving -1) hides shadow root children from the tab order.
The problem with #78 was that it removed the initial tabindex.

Do not leave `-1` when the element is re-enabled.
@notwaldorf notwaldorf merged commit 583e706 into PolymerElements:master Mar 7, 2018
@rileyjbauer
Copy link

rileyjbauer commented Jul 23, 2018

Hello!
I've read through the linked issues and PRs related to tabindex here because I am running into PolymerElements/paper-input#437 which seems to be caused by this logic within _disabledChanged.

In my case at least adding the line this.setAttribute('tabindex', -1); to _disabledChanged on line 112 (after this.tabIndex = -1;) fixed the issue that I was having where a disabled paper-input could still be tabbed to. Trying to programmatically update the tabindex as the input was enabled/disabled was failing because of _disabledChanged.

I understand that there's a lot of subtlety here, so before sending a PR, I wanted to ask if it was intentional that the actual tabindex attribute of the disabled paper-input element was not set to -1. Perhaps in order to allow tabbing to children of a disabled element? but why would that be desirable?

Please let me know if anything needs clarification. Thanks in advance!

@rslawik
Copy link
Contributor Author

rslawik commented Jul 27, 2018

Hi,
There should be no difference between this.tabIndex = -1; and this.setAttribute('tabindex', '-1');. Setting this.tabIndex = -1 sets the tabindex attribute to -1. I checked in Chrome.
Does it behave differently for you? Maybe there's even more subtlety here.

Could you point me to a demo showing the problem? Does it affect only <paper-input>? It will help investigate what the problem is.
Thanks.

@rslawik rslawik deleted the tabindex branch July 27, 2018 16:45
@rileyjbauer
Copy link

this.tabIndex = -1 sets the tabindex for the <input> within the paper-input, but not the paper-input itself.

this.setAttribute('tabindex', '-1') does set the tabindex for paper-input

Check out this codepen to see it better: https://codepen.io/rileybauer/pen/vaeMaQ

@rslawik
Copy link
Contributor Author

rslawik commented Jul 29, 2018

Thanks for the codepen! It was extremely helpful.

I investigated a little bit and filed PolymerElements/paper-input#667 so the <paper-input> owners can investigate further.

I think we can replace this.tabIndex = -1; with this.setAttribute('tabindex', '-1'); to avoid relying on the property and attribute to be in sync and treat the attribute as the source of truth.

@notwaldorf what do you think?

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

Successfully merging this pull request may close these issues.

4 participants