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

Inverts arrow key focus movement when in an RTL context. #41

Merged
merged 6 commits into from
Feb 2, 2016

Conversation

bicknellr
Copy link
Contributor

@bicknellr
Copy link
Contributor Author

Hmm, wct --sauce default seems to work.

@bicknellr
Copy link
Contributor Author

I think this needs Polymer/polymer#2717 also.

assert.equal(document.activeElement, menubar.items[2], 'document.activeElement is next item');
done();
// wait for async in _onFocus
}, 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

why 200 and not 100, or 1? it is waiting for an animation to finish, or just a microtask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I copied this from the other examples with the assumption that both actions would need to wait for the same thing because they both affect focus, so I wouldn't be surprised if the timing is too loose. I'll look more into this.

@notwaldorf
Copy link
Contributor

@bicknellr ping! what's the status of this? (also: tests are hella grumpy)

@bicknellr
Copy link
Contributor Author

update: Polymer/polymer#2717 is in as of not too long ago, but it isn't in a release yet so this is still blocked.

@bicknellr bicknellr force-pushed the bicknellr/paper-tabs_issue-85 branch from d27601b to befda3c Compare January 27, 2016 23:26
'`menubar.selected` should not change.');
done();
// wait for async in _onFocus
}, 200);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This comment is still applicable.)

@bicknellr
Copy link
Contributor Author

I took out all the setTimeout calls and it the tests still work as expected; I'm guessing it's because the events in these tests are all handled synchronously. Also, there's a few other tests (unrelated to this change other than the activeElement thing) that only use click and could probably go without setTimeout as well.

assert.equal(menubar.selected, 1,
'`menubar.selected` should not change.');

done();
Copy link
Contributor

Choose a reason for hiding this comment

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

no done needed anymore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@notwaldorf
Copy link
Contributor

LGTM % nit. Ship it! :shipit:

bicknellr added a commit that referenced this pull request Feb 2, 2016
…e-85

Inverts arrow key focus movement when in an RTL context.
@bicknellr bicknellr merged commit 05b5664 into master Feb 2, 2016
@bicknellr bicknellr deleted the bicknellr/paper-tabs_issue-85 branch February 2, 2016 02:19
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