Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

FocusTrapDirective in IE-11 throws runtime errors in some cases. #1587

Closed
krotscheck opened this issue Oct 12, 2017 · 6 comments
Closed

FocusTrapDirective in IE-11 throws runtime errors in some cases. #1587

krotscheck opened this issue Oct 12, 2017 · 6 comments
Assignees
Milestone

Comments

@krotscheck
Copy link
Contributor

Select one ... (check one with "x")

[ x ] bug
[ ] feature request
[ ] enhancement

Expected behavior

FocusTrapDirective.setPreviousFocus() should not error.

Actual behavior

FocusTrapDirective.setPreviousFocus() throws errors under certain conditions in IE-11 (see repro).

Reproduction of behavior

  1. In IE-11, document.activeElement can be null under certain circumstances (no form elements on page), or set to a DOM element type that does not support the focus() method in others (body).
  2. When the FocusTrapDirective's ngAfterViewInit is called, it thus grabs a reference to an element that focus cannot be passed back to.
  3. When that happens, calling setPreviousFocus() will throw an error.

Additional references: angular-ui/bootstrap#5096

Suggested fix: Add a bit of defensive coding:

    public setPreviousFocus(): void {
        if (this._previousActiveElement && this._previousActiveElement.focus) {
            this._previousActiveElement.focus();
        }
    }

Environment details

  • Angular version: 4.x.x

  • Clarity version: 0.10.8

  • OS and version: Windows

  • Browser: IE-11

@hippee-lee hippee-lee self-assigned this Oct 12, 2017
@hippee-lee hippee-lee added this to the 0.10.10 milestone Oct 12, 2017
@clementcur
Copy link
Contributor

It looks related to the fact that focus events in IE11 are handled asynchronously, thus after Angular component lifecycle, see jquery/jquery#3123

In our case, after confirming the modal panel, we navigate away from the previous view, and IE11 still tries to focus on the destroyed view...

@hippee-lee
Copy link
Contributor

@krotscheck , @clementcur - I'm unable to recreate a runtime error in an app for this issue.

In the movie below here is the code displayed on the page. I removed all navigation elements from the app for recreation purposes.

<p>
    Modal with no form inputs, buttons or a's.
    <span (click)="modalOpen = true">Open Modal</span>
</p>
<clr-modal [(clrModalOpen)]="modalOpen">
    <h3>Nice Title</h3>
    <div class="modal-body">
        <h4>And a span that navigates to to App Root routerLink</h4>
        <p><span [routerLink]="['../../']">Go web app root</span></p>
    </div>
</clr-modal>

Here are the steps I took to try and generate the error

  1. Navigate directly to http://localhost:4200/modal/modal-form
  2. Open the modal
  3. Navigate to the app root

In this test I have removed everything from the demo app (navigation, header etc) except for the code above and there is no form, form element, button or link on the page. The app does not generate an error.

Here is a screen capture of my attempt to recreate the issue as I understand it:
ie11_focustrap_issue_1587

I will talk to @youdz about it more as well but, I am hesitant to add guard code for an issue that I am unable to recreate / write a test for.

Can you advise on how to recreate this in IE11?

@clementcur
Copy link
Contributor

@hippee-lee We've seen it while running our unit tests against IE11 on Browserstack so far. We also noticed that the time spent to execute those tests takes 10x longer than other browsers, so we suspect there might be some race condition happening when the system/browser is being slow...

@hippee-lee
Copy link
Contributor

@clementcur - Do you have any ideas for good approach to recreating that in an app? Could it be something specific to how the test is scripted?

@krotscheck
Copy link
Contributor Author

krotscheck commented Oct 13, 2017

@hippee-lee I found a plunkr: http://jsfiddle.net/u3uNP/ - focus on a textbox in the bottom right, then click on the background. Note that the value of activeElement becomes the HTMLBodyElement, which in IE-11 does not support the focus() method.

You should be able to reproduce it in your test simply by setting activeElement = document.body, or blurring whatever element currently has focus. Note that the former might be tricky, as it's readOnly in most other browsers.

hippee-lee added a commit to hippee-lee/clarity that referenced this issue Oct 16, 2017
- Handles a use case for IE11 in some FocusTrapDirective useages
- Closes vmware-archive#1587

Signed-off-by: Matt Hippely <mhippely@vmware.com>
hippee-lee added a commit to hippee-lee/clarity that referenced this issue Oct 16, 2017
- Addresses use cases for IE11 where the previousActiveELement may not have the focus method
- Closes vmware-archive#1587

Signed-off-by: Matt Hippely <mhippely@vmware.com>
hippee-lee added a commit that referenced this issue Oct 16, 2017
- Addresses use cases for IE11 where the previousActiveELement may not have the focus method
- Closes #1587

Signed-off-by: Matt Hippely <mhippely@vmware.com>
@github-actions
Copy link

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants