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

Dialog.setAccessibleViewsVisible should be moved back to Joist #442

Closed
samreid opened this issue Jan 11, 2019 · 7 comments
Closed

Dialog.setAccessibleViewsVisible should be moved back to Joist #442

samreid opened this issue Jan 11, 2019 · 7 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 11, 2019

Discovered in #369. Dialog currently has:

    /**
     * Hide or show all accessible content related to the sim ScreenViews, navigation bar, and alert content. Instead
     * of using setVisible, we have to remove the subtree of accessible content from each view element in order to
     * prevent an IE11 bug where content remains invisible in the accessibility tree, see
     * https://github.com/phetsims/john-travoltage/issues/247
     *
     * @param {boolean} visible
     * @private
     */
    setAccessibleViewsVisible( visible ) {
      for ( let i = 0; i < this.sim.screens.length; i++ ) {
        this.sim.screens[ i ].view.accessibleVisible = visible;
      }
      this.sim.navigationBar.accessibleVisible = visible;
      this.sim.homeScreen && this.sim.homeScreen.view.setAccessibleVisible( visible );

      // workaround for a strange Edge bug where this child of the navigation bar remains visible,
      // see https://github.com/phetsims/a11y-research/issues/30
      if ( this.sim.navigationBar.keyboardHelpButton ) {
        this.sim.navigationBar.keyboardHelpButton.accessibleVisible = visible;
      }
    },

This leverages too much detail work in joist and should be encapsulated somewhere in Joist. Perhaps move setAccessibleViewsVisible to Sim.js? @jessegreenberg can you please take a look?

@jessegreenberg
Copy link
Contributor

Agreed. Furthermore, the workaround in this function is likely not required anymore after phetsims/a11y-research#30

@jessegreenberg
Copy link
Contributor

we have to remove the subtree of accessible content from each view element in order to prevent an IE11 bug where content remains invisible in the accessibility tree,

Same for this line, we aren't doing this anymore.

@jessegreenberg
Copy link
Contributor

And this should only be happening for modal Dialogs, it was a coincidence that Dialog only supports modal dialogs at the moment.

@jessegreenberg
Copy link
Contributor

Move to Sim.js was done in the above commit.

@jessegreenberg
Copy link
Contributor

I verified that the workaround is not necessary in Edge anymore.

@jessegreenberg
Copy link
Contributor

I thought about setAccessibleViewsVisible for non-modal dialogs briefly but decided not to go further until we support non-modal dialogs at all.

@samreid would you mind reviewing these changes?

@samreid
Copy link
Member Author

samreid commented Mar 6, 2019

Looks good to me, though I'm not sure how to test it. But I presume you have covered that part. Closing.

@samreid samreid closed this as completed Mar 6, 2019
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

2 participants