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

fix: check for activeElement before calling blur() on it #11260

Closed
wants to merge 2 commits into from
Closed

fix: check for activeElement before calling blur() on it #11260

wants to merge 2 commits into from

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented May 13, 2018

Description

Replaces #11214

Motivation and Context

Fixes the unit tests in IE11 on Windows 10.

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to change)

Checklist (all required):

  • I have read and follow the CONTRIBUTING document.
  • There are no other pull request similar to this one.
  • The pull request title is descriptive.
  • The template is fully and correctly filled.
  • The pull request targets the right branch (develop or support/*).
  • My commits are correctly titled and contain all relevant information.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).
  • All new and existing tests passed.

⚠️ Require:

@ncoden
Copy link
Contributor

ncoden commented May 13, 2018

Tests are subject to race conditions with this PR (as discussed in #11214). Waiting for #11259 to merge.

@DanielRuf
Copy link
Contributor Author

Mh, we still have the focus / blur issue on IE. Didn't we already fix this? On IE11 is is body.

#11074

@ncoden
Copy link
Contributor

ncoden commented May 16, 2018

See #11275

@ncoden
Copy link
Contributor

ncoden commented Jun 2, 2018

Tests pass with develop merged but there's an issue with BrowserStack. See browserstack/browserstack-runner#201

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Jun 14, 2018

Hm, even with the change Android 4.4 fails:
https://travis-ci.org/zurb/foundation-sites/jobs/392125882#L1133

@DanielRuf
Copy link
Contributor Author

I guess their devices are too slow.

@DanielRuf
Copy link
Contributor Author

@ncoden
Copy link
Contributor

ncoden commented Jun 14, 2018

@DanielRuf I already tried on your branch the changes with this PR #11333 and it works. There's just the browserstack runner bug.

@DanielRuf
Copy link
Contributor Author

@DanielRuf I already tried on your branch the changes with this PR #11333 and it works. There's just the browserstack runner bug.

What about the linked lines? Firefox and Android 4.4?

@DanielRuf
Copy link
Contributor Author

Well, then I'll wait and see what happens after the merge or rebase.

@ncoden
Copy link
Contributor

ncoden commented Jun 14, 2018

@DanielRuf They still happen time to time. I was only talking about the fails due to timeouts.

@ncoden
Copy link
Contributor

ncoden commented Jun 14, 2018

I made some tests and this PR (with a9334ad) introduce the following error:

[Windows 8.1, Firefox 60.0] Error: "closes the targeted submenu" failed, uncaught exception: AssertionError: expected { Object (0, length, ...) } to be hidden (:0)

See https://www.travis-ci.org/ncoden/foundation-sites/jobs/392158100

With a9334ad, tests always fails. The same branch with a9334ad never fails (I ran the tests 5 times)

@DanielRuf
Copy link
Contributor Author

With a9334ad, tests always fails. The same branch with a9334ad never fails (I ran the tests 5 times)

And IE11 does not fail then if we revert this? Initially we needed this for IE11 and a few other browsers due to the debouncing behavior and race conditions.

Revert it if this resolves the issues.

@ncoden
Copy link
Contributor

ncoden commented Jun 14, 2018

Initially we needed this because of:

TypeError: Die Eigenschaft "blur" eines undefinierten oder Nullverweises kann nicht abgerufen werden.

But I never seen this error in our tests. So unless you can reproduce it on browserstack and send me a link I think we can close this PR.

@ncoden
Copy link
Contributor

ncoden commented Jun 14, 2018

Racing conditions and tests for debouncing were resolved in others PRs. (#11279)

@DanielRuf DanielRuf closed this Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests: failing unit tests in IE11 due to blur() being called on null / undefined
2 participants