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

refactor(interactivity-checker.spec): remove redundant setTimeout usage #2159

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

belev
Copy link
Contributor

@belev belev commented Dec 10, 2016

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 10, 2016
@devversion
Copy link
Member

The reason why a native setTimeout was used here was that the delay comes from the browser itself and can't be flushed from a zone.

I just tried to run the tests locally without the tick or setTimeout and it seems to succeed anyways (not sure what flushes the queue)

let div = document.createElement('div'); document.body.appendChild(div); div.contentEditable = true; console.log(div.tabIndex);

@belev
Copy link
Contributor Author

belev commented Dec 10, 2016

Mhm interesting, maybe this should be closed then. However I will try to find why this happens.

@devversion
Copy link
Member

@belev Sounds good. Thank you!

@belev
Copy link
Contributor Author

belev commented Dec 10, 2016

After some digging around that's what I understood:

tabIndex attribute isn't set in the case where we have only contenteditable. Because of that hasValidTabIndex returns false and then getTabIndexValue returns null. So basically the tick or setTimeout didn't do anything in this case. And the test passes because of this line.

However if we access the tabIndex through the element it has value. So after all I think the mistake is in hasValidTabIndex function. I will think how to fix that.

What do you think @devversion ?

@devversion
Copy link
Member

devversion commented Dec 10, 2016

Ah I can see. The checks you mentioned are intentionally only respecting the user-specified tabindex instead of the evaluated one from the browser (here is a reference)

And as you said that's why the tests pass. We can remove the tick stuff safely here.

@belev belev changed the title refactor(interactivity-checker.spec): use tick instead of setTimeout refactor(interactivity-checker.spec): remove redundant setTimeout usage Dec 10, 2016
@belev
Copy link
Contributor Author

belev commented Dec 10, 2016

Made the change, you can review it again. : )

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn
Copy link
Member

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Dec 14, 2016
@jelbourn jelbourn merged commit 9e3c59c into angular:master Dec 14, 2016
@belev belev deleted the refine-unit-test branch December 14, 2016 20:56
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants