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.

Sign up for GitHub

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

Jump to bottom

Prevent the selected tab from being refocused when local DOM elements of the menu are focused. #36

Merged
merged 3 commits into from
Feb 9, 2016
Merged

Prevent the selected tab from being refocused when local DOM elements of the menu are focused. #36

merged 3 commits into from
Feb 9, 2016

Conversation

bicknellr
Copy link
Contributor

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch 2 times, most recently from 785baa9 to 1b97a7f Compare November 5, 2015 21:21
@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch from 1b97a7f to 6edd314 Compare November 5, 2015 22:39
cdata
cdata reviewed Nov 9, 2015
View reviewed changes
test/iron-menu-behavior.html
var menu = fixture('basic');
menu.extraContent.focus();
setTimeout(function() {
var menuActiveElement = Polymer.dom(menu.root).querySelector('*:focus');
Copy link
Contributor

Choose a reason for hiding this comment

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

:focus should work fine for this, FWIW.

@bicknellr
Copy link
Contributor Author

(needs Polymer/polymer#2717)

cdata
cdata reviewed Nov 16, 2015
View reviewed changes
iron-menu-behavior.html
// Do not focus the selected tab if the deepest target is part of the
// menu element's local DOM and is focusable.
var rootTarget = Polymer.dom(event).rootTarget;
if (rootTarget !== this && !this.isLightDescendant(rootTarget) && rootTarget.hasAttribute('tabindex')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be slightly faster to measure tabindex using the property syntax. Also on the subject of micro-optimization, it might be good to check for rootTarget.tabindex before this.isLightDescendant, so that we can short-circuit before the relatively more expensive check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@cdata
Copy link
Contributor

cdata commented Nov 16, 2015

Left some feedback, PTAL!

@bicknellr
Copy link
Contributor Author

Fixed up the stuff you mentioned.

@cdata
Copy link
Contributor

cdata commented Nov 30, 2015

@bicknellr please rebase this PR ⚾

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch from e76b170 to e3bc400 Compare November 30, 2015 20:55
@cdata
Copy link
Contributor

cdata commented Dec 1, 2015

Man, what happened to the test suite here? Looks like a lot of broken 💩 ..

@bicknellr
Copy link
Contributor Author

@cdata The tests use document.activeElement but assume there's no retargeting for shadow DOM boundaries, so I fixed them and made this PR dependent upon Polymer/polymer#2717.

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch from e3bc400 to b3ad13f Compare January 11, 2016 18:21
@bicknellr
Copy link
Contributor Author

Polymer/polymer#2717 is in. I wct --sauce default'd this with Polymer master and it passes, but that PR isn't in a release yet. Once it is, I'll bump the dependency version and the tests should start passing here.

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch from b3ad13f to 33ea777 Compare January 27, 2016 22:33
@bicknellr
Copy link
Contributor Author

@cdata, Polymer/polymer#2717 is now in a release.

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-65 branch from fd90fb1 to 529afb4 Compare February 2, 2016 02:37
@cdata
Copy link
Contributor

cdata commented Feb 9, 2016

Ship it! :shipit:

bicknellr added a commit that referenced this pull request Feb 9, 2016
@bicknellr
Merge pull request #36 from PolymerElements/bicknellr/paper-tabs_issu…
99c5d53 
…e-65

Prevent the selected tab from being refocused when local DOM elements of the menu are focused.
@bicknellr bicknellr merged commit 99c5d53 into master Feb 9, 2016
@bicknellr bicknellr deleted the bicknellr/paper-tabs_issue-65 branch February 9, 2016 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cdata cdata

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bicknellr @cdata @googlebot