Use ModalDialog in the notebook

[acelaya]

Depends on hypothesis/frontend-shared#1442 and hypothesis/frontend-shared#1443

Closes #6158

Use a ModalDialog for the notebook, which brings a few accessibility fixes like having role="dialog", aria-modal="true" and focus trap out of the box.

This addresses the next points from the accessibility review:

To make the dialog more accessible:

1. Add a role=”dialog” to the dialog container. This will allow assistive technologies to announce the dialog as it appears.
2. Add an aria-modal=”true” attribute to the dialog. This will hide background content from assistive technologies while the modal is open.
3. Shift keyboard focus into the dialog when it opens, and constrain focus within the modal until the user closes the dialog.

Testing steps

1. Run a screen reader
2. Open the notebook -> The screen reader should announce the opening of a dialog and mention the notebook frame
3. Press Tab -> Since the iframe is initially focused, it should focus the first element inside of it.

Known issues

The Modal's focus trap causes the focus to be trapped in the close button if you focus out of the iframe. I plan to address this separately as it seems a bit complex.

Considerations

While working on this PR I realized we are only hiding the notebook when closed, instead of fully removing it from the DOM.

At first I thought this was intentional, to avoid multiple loads of the iframe and its contents, but then I realized the iframe has a key prop which is intentionally updated every time it is opened, and there's a comment explaining that forces the content to be up to date.

Separately, I will check if we could avoid this extra piece of state by just fully removing the notebook from the DOM when closed, but I want to check if A) It doesn't have other side effects and B) it doesn't move us in the wrong direction, assuming the reloading of the iframe is just a workaround for something else, in which case current approach makes it more explicit that this is not the desired behavior long term.

EDIT: We have discussed the above and we are going to keep it as it is now, because the long-term intended approach is to not load the iframe at all if annotations didn't change. Destroying the notebook on close would make this desire less obvious.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5041947) 99.44% compared to head (203276f) 99.44%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6162   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         264      264           
  Lines       10051    10055    +4     
  Branches     2386     2387    +1     
=======================================
+ Hits         9995     9999    +4     
  Misses         56       56           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acelaya]

I considered moving this to the ModalDialog classes, but since we are not removing it from the DOM but just hiding it, that left the backdrop still visible.

[robertknight]

A few things I noticed when testing:

• With Chrome and VoiceOver, opening the notebook from "Open Notebook" in the client for the first time focuses the iframe and announces the user is in a dialog as expected
• If the user opens the notebook for a second time, focus does not move into the iframe however
• After tabbing out of the notebook iframe, focus remains trapped on the "close notebook" button, and it isn't possible to tab back into the notebook frame
• In Safari, the notebook iframe is not focused the first time it opens. I believe this is a known issue that also affects the sidebar and so I think we can discount it here.

[acelaya]

A few things I noticed when testing:

• With Chrome and VoiceOver, opening the notebook from "Open Notebook" in the client for the first time focuses the iframe and announces the user is in a dialog as expected
• If the user opens the notebook for a second time, focus does not move into the iframe however
• After tabbing out of the notebook iframe, focus remains trapped on the "close notebook" button, and it isn't possible to tab back into the notebook frame
• In Safari, the notebook iframe is not focused the first time it opens. I believe this is a known issue that also affects the sidebar and so I think we can discount it here.

Hhmm, I think the focus issues and the different behavior between first time and following times is related with what I mention in the "Considerations" section of the PR description.

Since the modal itself is always present in the DOM and just hidden/shown, screen readers probably don't consider it a new element to announce.

I'm going to try and verify this. If that's the case, I'll create another PR trying to fix that first.

[acelaya]

• With Chrome and VoiceOver, opening the notebook from "Open Notebook" in the client for the first time focuses the iframe and announces the user is in a dialog as expected
• If the user opens the notebook for a second time, focus does not move into the iframe however

I can confirm this issue is caused by not removing the modal from the DOM when we close it.

EDIT: To be more precise, it is caused by the fact that ModalDialog's initialFocus assumes the modal element is destroyed when closed, and therefore, the element to focus initially is "new", so perhaps we need to implement a custom way to focus the iframe which is not based in ModalDialog's initialFocus.

[acelaya]

• After tabbing out of the notebook iframe, focus remains trapped on the "close notebook" button, and it isn't possible to tab back into the notebook frame

After a lot of debugging, I have realized this is caused by the Modal's built-in focus trap functionality.

This internally uses the useTabKeyNavigation hook, which queries elements inside the modal to manually focus and handle tab indexes on them.

The query uses a selector which is a,button:not([role="tab"]),input,select,textarea,[role="grid"],[role="tablist"] by default.

If I add iframe there, then the focus no longer gets trapped in the close button, but it breaks the modal's focus trap and allows you to focus elements which are outside the modal. I guess it's because once you have focused the iframe, you can focus elements inside of it, and the manual handling of tab indexes gets out of sync with the actually focused element.

I'll have to dig a bit further on this, and potentially tweak the useTabKeyNavigation hook, so maybe it makes sense to defer this to a separated PR for simplicity.

[acelaya]

I'm not always attaching to the DOM, because it breaks other tests, and is relevant only to test the default focusing functionality.

[robertknight]

If I add iframe there, then the focus no longer gets trapped in the close button, but it breaks the modal's focus trap and allows you to focus elements which are outside the modal. I guess it's because once you have focused the iframe, you can focus elements inside of it, and the manual handling of tab indexes gets out of sync with the actually focused element.

I'd like us to understand exactly what is happening here. Is it something that could affect other elements?

[acelaya]

If I add iframe there, then the focus no longer gets trapped in the close button, but it breaks the modal's focus trap and allows you to focus elements which are outside the modal. I guess it's because once you have focused the iframe, you can focus elements inside of it, and the manual handling of tab indexes gets out of sync with the actually focused element.

I'd like us to understand exactly what is happening here. Is it something that could affect other elements?

The way useTabKeyNavigation works is by querying all the elements inside the modal that would usually be focusable, and it sets tabIndex={-1} to all of them, so that they can no longer be normally focused.

Then, it captures the keydown event on the modal to check if Tab was pressed, and if so, it checks what's the "next" natural element to focus (by tracking indexes), and sets tabIndex={0} to it and then focuses it (via element.focus()).

This keeps the focus trapped inside the modal while it is opened.

The first problem with this is that this logic never took into account that an iframe could be rendered inside the modal, and the default selector to find all focusable elements does not include iframes.

The second problem is that, even if we include it, once the focus has transitioned inside the iframe itself, the logic in the hook stops working, probably because the manual tabIndex-setting and element.focusing is no longer in sync.

There is a bit of speculation in my explanation, and I will have to dig deeper to know all the details, but this seems to be more or less the problem.

[acelaya]

I've been doing some more testing with the ModalDialog in frontend-shared pattern library, and it's more or less what I described above. Not exactly that anything is getting "out of sync", but the fact that the keydown handler in the modal itself, no longer triggers, as soon as the focus goes to a different component, "disabling" the logic from useTabKeyNavigation.

I have realized that the focus trap works because the modal renders an overlay that doesn't allow you to click anywhere else and incidentally focus other elements outside of the modal, but if you remove the overlay and click somewhere else, you can reproduce the same issue happening with the iframe.

If we render an iframe inside the modal itself, there's no way to prevent the user from clicking "outside" the modal, which ends up with a tab sequence that includes the rest of the content from the main frame which is outside (or "behind") the modal.

[acelaya]

I'm exploring some alternative approach to do focus trapping that allows focusing the iframe and its content, while preventing the elements outside the modal to be focused.

I'll put this on hold for now.

[acelaya]

Closing in favor of #6187