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

DanielRuf · 2018-04-29T18:31:53Z

Description

Currently the tests throw errors in IE11 on Windows 10 because there might be some race conditions or differences in the browser behaviors as document.activeElement is not always defined.

Closes tests: failing unit tests in IE11 due to blur() being called on null / undefined #11213

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):

ncoden

Stupid mistake on my side, sorry about that.

But I don't understand why doesn't this appear in https://travis-ci.org/zurb/foundation-sites/jobs/372717914 ? I can reproduce it locally with index.html.

ncoden · 2018-04-29T18:40:09Z

test/javascript/components/accordion.js

@@ -27,7 +27,7 @@ describe('Accordion', function() {

  afterEach(function() {
    plugin.destroy();
-    document.activeElement.blur();
+    if(document.activeElement)document.activeElement.blur();


if(document.activeElement)document.activeElement.blur(); // ^ // no space ?

DanielRuf · 2018-04-29T18:41:32Z

I still guess that the tests on Browserstack run slower than on real devices.

ncoden · 2018-04-29T18:46:40Z

chore: add missing space between condition and statement

Prefix depends on the type of change, not where it is. Here, it should be style

ncoden · 2018-04-29T18:48:53Z

test/javascript/util/keyboard.js

@@ -20,7 +20,7 @@ describe('Keyboard util', function() {
  };

  afterEach(function() {
-    document.activeElement.blur();
+    if(document.activeElement)document.activeElement.blur();


if(document.activeElement)document.activeElement.blur(); // ^ // no space ?

You can rebase this change with the previous commit I guess. The PR is clear enough. And use style: ...

ncoden

29d21c8 tests: always call blur() for asynchronous calls

Why "asynchronous calls". I see nothing asynchronous in test/javascript/components/dropdownMenu.js.

Also even if .focus() is called asynchronously, we wait for the test to finish before calling .blur() in afterEach.

So this line is not the issue.

ncoden · 2018-04-30T07:39:26Z

test/javascript/components/interchange.js

-
-      setTimeout(function() {
-        $(window).trigger('resize');
-      }, 20);


This break the purpose of the test itself. calls reflow on viewport size change once means that with multiple resize, _reflow is called only once within a given delay (10ms by default for MutationObserver debounce). We cannot test this behavior without triggering resize once.

So we actually broke these test before with switching to delays over 10ms. We should triggers several resize in 10ms and expect reflow to be called once.

Also, this should not be in this PR.

Right. But not fixing this in this PR will produce a failing Travis build.

What do you recommend because of this issue with reflow and the timing issues on the Travis containers?

Right. But not fixing this in this PR will produce a failing Travis build.

Are you sure ? Tests runs after tests: always call blur() for asynchronous calls. Do we need both commits for tests to runs ?

I would recommend to actually test what we want to test and expect it to fail if the component does not work properly: trigger resize several times in 10ms and check after that if _reflow was called once.

This shoud work. If it doesn't, Interchange has a bug. I'll take a look at this later.

I'll take a look at this later.

This would be really helpful, thanks.

DanielRuf · 2018-04-30T07:51:33Z

So this line is not the issue.

The testsuite tells differently but ok. How would you solve this here? Not sure if we can solve these issues related to timing functions when the browser / VM is too slow.

ncoden · 2018-05-11T21:34:06Z

I'll take a look at this later.

Will do this tomorrow

ncoden · 2018-05-13T10:28:02Z

So the bug with the previous Interchange tests came from the browser not able to respect the setTimeout delays, which lead to big differences between several timeout running in parrallel. If the difference is bigger than the utils.trigger debounce time (10ms), several reflow may be triggered.

DanielRuf · 2018-05-13T10:36:59Z

Well, in general setTimeout is never exact when it comes to timing as it is often longer than what is set as timeout.

So what is the correct solution in your opinion? Keep in mind that the VM might be slower and timeouts may trigger later.

ncoden · 2018-05-13T15:25:07Z

I have a working solution. I'll open a PR specific to it because it require a bug fix in utils.trigger

ncoden · 2018-05-13T20:11:32Z

See #11259

If you're okay with these PRs, I guess you can create a new clean PR to replace this one without these commits:

9dbb994 tests: trigger resize event in interchange test only once

29d21c8 tests: always call blur() for asynchronous calls

2caa5dd tests: decrease timeout delay

DanielRuf · 2018-05-13T20:26:20Z

If you're okay with these PRs

Which PRs? You linked just one? I thought these replace mine?

ncoden · 2018-05-13T20:27:48Z

Sorry, see #11259 and its required fix #11258.

They only partially replace this PR as they only fix the Interchange test issue. The if(document.activeElement) fix is still required but I think we should propose it in a new PR (that would fully replace this one) and let this PR git history untouched.

Like "As soon as a Git branch is public, active and used, its commit history must be kept intact".

ncoden · 2018-05-13T20:43:44Z

Closed in favor of #11260

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

d3f687e

DanielRuf added the 🚨tests label Apr 29, 2018

ncoden reviewed Apr 29, 2018

View reviewed changes

DanielRuf added 4 commits April 29, 2018 21:03

style: add missing space between condition and statement

decd050

tests: trigger resize event in interchange test only once

9dbb994

tests: always call blur() for asynchronous calls

29d21c8

tests: decrease timeout delay

2caa5dd

ncoden added the WIP label Apr 29, 2018

ncoden suggested changes Apr 30, 2018

View reviewed changes

ncoden self-assigned this May 1, 2018

ncoden mentioned this pull request May 13, 2018

test: minimize interchange debouce test race condition #11259

Merged

14 tasks

DanielRuf mentioned this pull request May 13, 2018

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

Closed

14 tasks

DanielRuf closed this May 13, 2018

DanielRuf deleted the tests/blur-check-active-element branch May 13, 2018 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DanielRuf commented Apr 29, 2018

ncoden left a comment

ncoden Apr 29, 2018 •

edited

Loading

DanielRuf commented Apr 29, 2018

ncoden commented Apr 29, 2018

ncoden Apr 29, 2018

ncoden left a comment

ncoden Apr 30, 2018

DanielRuf Apr 30, 2018

ncoden Apr 30, 2018

DanielRuf Apr 30, 2018

DanielRuf commented Apr 30, 2018

ncoden commented May 11, 2018

ncoden commented May 13, 2018

DanielRuf commented May 13, 2018

ncoden commented May 13, 2018

ncoden commented May 13, 2018 •

edited

Loading

DanielRuf commented May 13, 2018

ncoden commented May 13, 2018 •

edited

Loading

ncoden commented May 13, 2018

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

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

Conversation

DanielRuf commented Apr 29, 2018

Description

Motivation and Context

Types of changes

Checklist (all required):

ncoden left a comment

Choose a reason for hiding this comment

ncoden Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

DanielRuf commented Apr 29, 2018

ncoden commented Apr 29, 2018

ncoden Apr 29, 2018

Choose a reason for hiding this comment

ncoden left a comment

Choose a reason for hiding this comment

ncoden Apr 30, 2018

Choose a reason for hiding this comment

DanielRuf Apr 30, 2018

Choose a reason for hiding this comment

ncoden Apr 30, 2018

Choose a reason for hiding this comment

DanielRuf Apr 30, 2018

Choose a reason for hiding this comment

DanielRuf commented Apr 30, 2018

ncoden commented May 11, 2018

ncoden commented May 13, 2018

DanielRuf commented May 13, 2018

ncoden commented May 13, 2018

ncoden commented May 13, 2018 • edited Loading

DanielRuf commented May 13, 2018

ncoden commented May 13, 2018 • edited Loading

ncoden commented May 13, 2018

ncoden Apr 29, 2018 •

edited

Loading

ncoden commented May 13, 2018 •

edited

Loading

ncoden commented May 13, 2018 •

edited

Loading