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

Edge bug: child of hidden element not added to DOM when parent is un-hidden #30

Closed
jessegreenberg opened this issue Apr 11, 2017 · 10 comments

Comments

@jessegreenberg
Copy link
Contributor

This bit me hard today - still not sure what is going on here. phetsims/john-travoltage#221 was the original issue.

When the "Keyboard Help Dialog" is opened with the keyboard, the "Keyboard Help Button" is removed from the focus order in Edge.

The parent of the button is the footer and it gets hidden correctly when the dialog opens. But when we try to set focus to the keyboard help button again, Edge won't focus it, and the button remains out of navigation order. Neither the button nor any of its ancestors is hidden or aria-hidden or has tabindex=-1.

It is almost like Edge isn't adding the button back into the DOM after removing the hidden attribute from the parent element. Explicitly setting hidden to false on the child elements fixes the problem.

If we can reproduce in a test case we should submit a bug report to Edge.

@jessegreenberg
Copy link
Contributor Author

I encountered this again in phetsims/coulombs-law#45.

It is almost like Edge isn't adding the button back into the DOM after removing the hidden attribute from the parent element.

This still feels like an accurate description, unhidden elements are not added back to the navigation order unless they are explicitely focused with focus()

@jessegreenberg
Copy link
Contributor Author

A timeout is a workaround for this bug for some reason.

@jessegreenberg
Copy link
Contributor Author

The above commit is breaking unit test so we can't continue to use it. The problem (obvious in hindsight) is that when a node is made invisible to AT, it remains in the focus order for long enough to stay focusable in the same callback.

@jessegreenberg
Copy link
Contributor Author

Should be fixed in the above commit, we will see if CT clears up.

@jessegreenberg
Copy link
Contributor Author

The timeout is causing focus to be lost in Edge. Lets hit the drawing board.

@jessegreenberg jessegreenberg self-assigned this Apr 5, 2018
@jessegreenberg
Copy link
Contributor Author

I discovered that forcing the browser to redraw the PDOM is a workaround for this bug in Edge. I added this block:

      // Edge has a bug where removing the hidden attribute doesn't add elements back to the navigation order. As a
      // workaround, forcing the browser to redraw the PDOM seems to fix the issue. Forced redraw method recommended by
      // https://stackoverflow.com/questions/8840580/force-dom-redraw-refresh-on-chrome-mac
      if ( platform.edge ) {
        this.display.getAccessibleDOMElement().style.display = 'none';
        this.display.getAccessibleDOMElement().style.display = 'block';
      }

and problem goes away. This is much better than a timeout workaround.

@jessegreenberg
Copy link
Contributor Author

Done with above commit, it is working well. Removing my assignment.

@jessegreenberg
Copy link
Contributor Author

This workaround has been working well for about 9 months and we have no plans to work on this any more. Closing.

@jessegreenberg
Copy link
Contributor Author

Edge is now in Chromium and this workaround may not be necessary anymore. It would be nice to remove because of phetsims/phet-core#115

@jessegreenberg
Copy link
Contributor Author

I removed the workaround and I verified that we are no longer seeing the issue mentioned in phetsims/john-travoltage#221. It seems very unlikely this is still a bug for chromium based Edge and I think this can be re-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant