[Transition] Fix hidden children appearing in a11y tree

[eps1lon]

• make element requirement in transitions explicit (we always used cloneElement so that was always implicated)
• set aria-hidden and visibility: hidden if exited => children are not part of the a11y tree and not focusible

I observed that the code variant switches in our docs were still focusable. This is due to Fade simply setting opacity to 0 which has not effect on the a11y tree.

Videos taken in chrome 72

production:

PR:

Behavior verified in latest chrome, firefox, edge and IE 11 (safari is not working in browserstack).

By setting the hidden html prop they should automatically disappear but this will set display: none by spec. However by setting display: initial the children are focusable again. Only by setting visibility: hidden do we achieve that we keep the previous layout behavior and remove the content from the a11y tree.
Sets the aria-hidden property for screenscreaders and sets visibility to hidden to remove the childs from the tab order.

Nice side effect: In Concurrent React the hidden attribute will cause React to render the content with lower priority. We can't use hidden because that will cause display: none. This was the original implementation but display: initial is not supported in IE 11

Also removed some redundant prop type validation for children and marked it as required similar to #14444

Going to add this to the other transitions. Would close #12632

[eps1lon]

Failing test are IMO expected. If you don't want to mount if in={false} then react-transition-group provides mountOnEnter={false}

[oliviertassinari]

We have a problem with the focus logic. A visibility hidden element can't receive the focus. Now, the problem is that our Modal, Dialog, Menu components don't wait for the transition to start to focus the children, hence it's not working. The Firefox 52 test fail is just a symptom, you could see that the dialog autofocus logic was broken too on Chrome latest. There is no quick win here, it's a difficult problem to solve. I think that we can think of a solution handle:

• The focus accessibility issue of [Transition] Fix hidden children appearing in a11y tree #14465
• The API Cant use Transition Group render prop api. Prop-types incorrect #14376
• Some transition components would be better off with visibility hidden while some other with display none. We need to figure it out.

[eps1lon]

We have a problem with the focus logic. A visibility hidden element can't receive the focus. Now, the problem is that our Modal, Dialog, Menu components don't wait for the transition to start to focus the children, hence it's not working. There is no quick win here, it's a difficult problem to solve.

I think the issue is that those components set the in prop to true while simultaneously using an imperative focus in the DOM. They would need to wait for the changes to be commited to the DOM and then focus i.e. set the prop in the render phase and then focus in e.g. useEffect. Currently we set the prop and focus during render which is causing the issue.

The good thing is that this was always an issue that might've caused some subtle bugs. The bad thing is that we also need to fix dependent components.

[eps1lon]

github UI is broken for this PR. It shows 41 commits and 600 files changed while compare correctly shows only 23 commits and 12 files changed. Going to squash it. History is saved locally.

[eps1lon]

Quick update regarding FF 52 focus issue: It does work properly in browserstack and fiddling with https://deploy-preview-14465--material-ui.netlify.com/demos/menus/#simple-menu.

I suspect in the test the menu is still hidden. Other browsers can focus that content while FF cannot.

[oliviertassinari]

@eps1lon It's not related to Firefox, it's broken for everybody: #14465 (comment).

[eps1lon]

@eps1lon It's not related to Firefox

I was speaking about the build failure. What do you mean?

I did not understand that comment:

A visibility hidden element can't receive the focus.

This is intended. This is the purpose of this PR: Hidden elements should not be focusable.

Now, the problem is that our Modal, Dialog, Menu components don't wait for the transition to start to focus the children, hence it's not working.

They don't transition into aria-hidden=false. It's not hidden as soon as the in=true.

Could you explain what is currently not working? Elements seem to receive focus just fine when manually testing keyboard focus on https://deploy-preview-14465--material-ui.netlify.com/demos/menus/#simple-menu

[oliviertassinari]

[eps1lon]

@oliviertassinari What browser?

Debug notes (limited to Menu.test.js should focus the first item as nothing has been selected:

console.log(document.querySelector('[data-mui-test="Popover"]')) after wrapper.setState({ open: true });:
ChromeHeadless:

<div
  class="MuiPaper-root-7 MuiMenu-paper-1 MuiPaper-elevation8-17 MuiPaper-rounded-8 MuiPopover-paper-2"
  data-mui-test="Popover"
  aria-hidden="false"
  role="document"
  tabindex="-1"
  style="opacity: 1; transform: scale(1, 1); transition: opacity 0ms cubic-bezier(0.4, 0, 0.2, 1) 0ms, transform 0ms cubic-bezier(0.4, 0, 0.2, 1) 0ms; top: 16px; left: 16px; transform-origin: -8px -8px;"
>
  <ul
    class="MuiList-root-34 MuiList-padding-35"
    role="menu"
    data-mui-test="Menu"
  >
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="0"
      role="menuitem"
    >
      Menu Item 1<span class="MuiTouchRipple-root-56" />
    </li>
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="-1"
      role="menuitem"
    >
      Menu Item 2<span class="MuiTouchRipple-root-56" />
    </li>
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="-1"
      role="menuitem"
    >
      Menu Item 3<span class="MuiTouchRipple-root-56" />
    </li>
  </ul>
</div>;

Firefox 52:

<div
  class="MuiPaper-root-7 MuiMenu-paper-1 MuiPaper-elevation8-17 MuiPaper-rounded-8 MuiPopover-paper-2"
  data-mui-test="Popover"
  aria-hidden="false"
  style="opacity: 1; transform: scale(1, 1); transition: opacity 0ms cubic-bezier(0.4, 0, 0.2, 1) 0ms, transform 0ms cubic-bezier(0.4, 0, 0.2, 1) 0ms; top: 16px; left: 16px; transform-origin: -8px -8px 0px;"
  role="document"
  tabindex="-1"
>
  <ul
    class="MuiList-root-34 MuiList-padding-35"
    role="menu"
    data-mui-test="Menu"
  >
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="0"
      role="menuitem"
    >
      Menu Item 1<span class="MuiTouchRipple-root-56" />
    </li>
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="-1"
      role="menuitem"
    >
      Menu Item 2<span class="MuiTouchRipple-root-56" />
    </li>
    <li
      class="MuiButtonBase-root-53 MuiListItem-root-41 MuiListItem-default-44 MuiListItem-gutters-49 MuiListItem-button-50 MuiMenuItem-root-38 MuiMenuItem-gutters-39"
      tabindex="-1"
      role="menuitem"
    >
      Menu Item 3<span class="MuiTouchRipple-root-56" />
    </li>
  </ul>
</div>;

only diff at the possition of the inline style attribute.

However Firefox has the element with the className MuiPaper-root-7 MuiMenu-paper-1 MuiPaper-elevation8-17 MuiPaper-rounded-8 MuiPopover-paper-2 in document.activeElement.

[eps1lon]

Firefox issue is resolved. Investigating why opening a Dialog does not set focus on the dialog. It was working for Menu which was shown in the resolved firefox issue.

[eps1lon]

The issue was that the visibility was hidden for one render cycle in which we changed to in prop from false to true. This causes the modal to handle focus but react-transition-group#Transition will have one cycle where the state is still exited. After that it will switch to entering.

3887ae4 added additional logic to handle this case. aria-hidden is not needed anymore. I originally thought this was enough but aria-hidden does not affect how focus in child elements is handled. Only display and visibility can affect child elements.

@oliviertassinari Could you verify if the issue you reported still exists?