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

guard against null keyEventTarget #48

Merged
merged 5 commits into from
Jun 24, 2016

Conversation

dschuyler
Copy link
Contributor

There are two calls to _listenKeyEventListeners() and one already guards against null this.keyEventTarget. This change guards the call to _listenKeyEventListeners in attached from using a null keyEventTarget.

There are two calls to _listenKeyEventListeners() and one already guards against  null this.keyEventTarget. This change guards the call to _listenKeyEventListeners in attached from using a null keyEventTarget.
@dschuyler
Copy link
Contributor Author

/cc @valdrinkoshi, @dbeam

@dschuyler
Copy link
Contributor Author

It may be worth moving the keyEventTarget check into _listenKeyEventListeners() rather than checking keyEventTarget before calling _listenKeyEventListeners(). WDYT?

Moving the keyEventTarget check into _listenKeyEventListeners()
this._listenKeyEventListeners();
}
},

_listenKeyEventListeners: function() {
if (!this.keyEventTarget) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a much better place for this check! 🍻

@valdrinkoshi
Copy link
Member

valdrinkoshi commented Jun 24, 2016

Thanks for the contribution @dschuyler!
As per our guidelines for contributions, we require to file an issue and mention it in the description of the PR (e.g. Fixes #issue_number) and write at least one unit test to avoid any future regression.
Could you please do these?
It seems the problem was caused by the fact that one could set keyEventTarget = null before the element is attached.

@dschuyler
Copy link
Contributor Author

Fixes #49

var element = document.createElement('x-a11y-basic-keys');
element.keyEventTarget = null;
Polymer.dom(container).appendChild(element);
done();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: since the test is synchronous, you don't need to have to call the done callback (remove it from here and from line 444)

@valdrinkoshi
Copy link
Member

LGTM, thanks for your contribution @dschuyler!

@valdrinkoshi valdrinkoshi merged commit 5f77227 into PolymerElements:master Jun 24, 2016
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.

None yet

3 participants