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

chore: clean components & focus after tests #11210

Merged
merged 4 commits into from
Apr 29, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Apr 27, 2018

Description

After tests are run, any HTML added must be removed as the DOM is shared with others tests. Otherwise, there can be side effects on others tests.

This is what happened with DropdownMenu, where one drodown that was not removed made an other tests failing by changing the way the browser should handle the focus.

Changes

  • ensure html and components are cleared after DropdownMenu tests.
    They was not cleared as $html and plugin were re-defined in tests, overriding already created $html and plugin and making them not being cleared.
  • removed toggler triggers after Toggler tests
  • clear focus after all tests involving focus

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.

@DanielRuf
Copy link
Contributor

where one drodown that was not removed made an other tests failing by changing the way the browser should handle the focus.

Thought https://github.com/ncoden/foundation-sites/blob/814d2e55f2313675c558d2d520b423bf02a79b4e/test/javascript/components/dropdownMenu.js#L45 already does this.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 27, 2018

Thought ncoden/foundation-sites:test/javascript/components/dropdownMenu.js@814d2e5#L45 already does this.

Updated my PR description:

Changes

  • ensure html and components are cleared after DropdownMenu tests.
    They was not cleared as $html and plugin were re-defined in tests, overriding already created $html and plugin and making them not being cleared.
  • removed toggler triggers after Toggler tests

@DanielRuf
Copy link
Contributor

DanielRuf commented Apr 27, 2018

@ncoden
Copy link
Contributor Author

ncoden commented Apr 27, 2018

This does not fix it and produces new issues as far as I can see.

True. I don't know why I got this working when testing... I'll investigate further.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 27, 2018

According to me, the "works well alone but not after others tests" thing indicates that something is not cleared correctly, in tests or components.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 27, 2018

Edit: The bug seems to be caused by the test just above:

Dropdown: keyboard events Dropdown does not focus Dropdown when anchor is an input 

Without this test, everything works fine. With this test only, we get the focus bug.

screen shot 2018-04-28 at 00 44 02

@ncoden ncoden changed the title chore: clean DropdownMenu & Toggler components after tests chore: clean components & focus after tests Apr 28, 2018
@ncoden
Copy link
Contributor Author

ncoden commented Apr 28, 2018

Focus caused by others tests was the problem. IE seems to not handle it correctly when an element is focused, then removed and an other element is focused, all of that synchronously.

If we add a timeout just before the second element is focused, or if we clear the focus before the first element is removed, the second element is focused correctly.

I really think this is a browser issue because the Foundation components involved in this issue (Dropdown and DropdownMenu) do not rely on the currently focused element (no :focus or activeElement used) and because no .focus() except the one we expect is called during the test.

Tests runs well now and I don't think there is anything we can change in the component itself.

@DanielRuf What do you think ?

@ncoden
Copy link
Contributor Author

ncoden commented Apr 28, 2018

[Windows 7, Internet Explorer 9.0] Error: "does not find focusable elements with negative tabindex" failed, expected {} to equal 0
(...)
-- https://travis-ci.org/zurb/foundation-sites/jobs/372449224#L1236

This was not introduced by this PR. See tests for for develop: https://travis-ci.org/zurb/foundation-sites/jobs/372198260#L4883

@ncoden
Copy link
Contributor Author

ncoden commented Apr 28, 2018

@DanielRuf

Do we not need the comment here?

See #11210 (comment). Do you want me to add these comments back ?

@DanielRuf
Copy link
Contributor

See #11210 (comment). Do you want me to add these comments back ?

Probably as these describe what happens there.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 29, 2018

Probably as these describe what happens there.

9d39644 docs: add back comments in dropdownMenu tests

@ncoden ncoden merged commit 60853d9 into foundation:develop Apr 29, 2018
@ncoden ncoden added this to the 6.5.0 milestone Apr 29, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…-after-tests for v6.5.0

b7ccd98 chore: clean component after DropdownMenu tests foundation#11074
814d2e5 chore: clean component after Toggler tests
f9b61ff chore: clean focus after tests involving focus
9d39644 docs: add back comments in dropdownMenu tests

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
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.

[Dropdown] tests: activeElement is the body in IE11
2 participants