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

Display notebook in a native dialog #6187

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Display notebook in a native dialog #6187

merged 1 commit into from
Feb 8, 2024

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Feb 7, 2024

Closes #6158

This PR is an alternative to #6162, which tries to use a native dialog element to display the notebook.

The benefit of this is that it requires very little changes and solves role="dialog", aria-modal="true" and focus trapping out of the box.

The main drawback is that there are older versions of browsers we still support which don't support dialog. In those cases we will continue rendering the old less accessible markup, until all browsers we support can use dialog elements.

The good thing is that less-than-two-years-old browsers support dialog, so that will be the triggered branch in most of the cases.

Testing steps

On a modern browser, the notebook should look the same, but focus should be trapped, and pressing Tab should never focus an element outside of it.

To test old browsers behavior, open NotebookModal and edit isModalDialogSupported:

function isModalDialogSupported(document: Document) {
+  return false;
-  const dialog = document.createElement('dialog');
-  return typeof dialog.showModal === 'function';
}

With this, the notebook should look and behave as in main branch.

TODO

  • Add/fix tests
  • Fall back to the old markup for browsers not supporting dialog.
    function isModalDialogSupported() {
      const dialog = document.createElement('dialog');
      return typeof dialog.showModal === 'function';
    }
  • Determine why is a margin rendered on the dialog and "take control" over it -> See Display notebook in a native dialog #6187 (comment)

@acelaya acelaya force-pushed the notebook-native-dialog branch 2 times, most recently from bacdd8f to 5f3dc9c Compare February 8, 2024 09:25
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c19a5a2) 99.44% compared to head (ca352d3) 99.44%.

❗ Current head ca352d3 differs from pull request most recent head 65b196a. Consider uploading reports for the commit 65b196a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6187   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files         266      266           
  Lines       10086    10108   +22     
  Branches     2391     2395    +4     
=======================================
+ Hits        10030    10052   +22     
  Misses         56       56           

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

@acelaya acelaya force-pushed the notebook-native-dialog branch 2 times, most recently from d70dfc8 to d7b69f8 Compare February 8, 2024 09:49
@acelaya acelaya force-pushed the notebook-native-dialog branch 3 times, most recently from 7d05f12 to 5b7daab Compare February 8, 2024 10:16
@acelaya acelaya marked this pull request as ready for review February 8, 2024 10:21
src/annotator/components/NotebookModal.tsx Outdated Show resolved Hide resolved
src/annotator/components/test/NotebookModal-test.js Outdated Show resolved Hide resolved
src/annotator/components/test/NotebookModal-test.js Outdated Show resolved Hide resolved
src/annotator/components/NotebookModal.tsx Outdated Show resolved Hide resolved
src/annotator/components/test/NotebookModal-test.js Outdated Show resolved Hide resolved
src/annotator/components/NotebookModal.tsx Outdated Show resolved Hide resolved
src/annotator/components/NotebookModal.tsx Outdated Show resolved Hide resolved
src/annotator/components/NotebookModal.tsx Outdated Show resolved Hide resolved
src/annotator/components/test/NotebookModal-test.js Outdated Show resolved Hide resolved
@acelaya acelaya merged commit 0053159 into main Feb 8, 2024
2 checks passed
@acelaya acelaya deleted the notebook-native-dialog branch February 8, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Notebook modal dialog is not properly represented as a dialog
2 participants