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

feat(dialog): use native HTML dialog element #1865

Closed

Conversation

adamjohnson
Copy link
Collaborator

@adamjohnson adamjohnson commented Sep 13, 2024

What I did

  1. Change internal structure of rh-dialog to use the native HTML <dialog> element.
  2. Overhaul much of the CSS.
  3. Change how we handle #overlay to use the CSS native ::backdrop psuedo-element.
  4. Left notes about deprecating #overlay
  5. Added the accessible-label property for when users want to manually label their modals + added docs.
  6. Modal now has logic to decide when to use accessible-label, aria-labelledby + IDREF, and the aria-label of the trigger element.
  7. Improved layout of modal contents when scrollbars are present
  8. Increased the touch target of the close modal button.
  9. Trap focus inside the modal for improved accessibility (WIP).

Testing Instructions

  1. Visit the DP demo.
  2. Add all kinds of content to the modal header, body, and footer, see if you can break it.
  3. See if focus trapping works with various different interactive elements (or none at all).

To Do's

  • Fix Shift + Tab focus trapping
  • Fix focus trap with Tab in video modal with YouTube embed
  • Update/improve changeset
  • Test across different browsers and AT
  • Look into why trigger/rh-button displays two buttons (one without a label) in the DP but not locally

Notes to Reviewers

Some docs on the Guidelines page are incorrect as they have the modal body scrolling independently from the header and footer. That needs updated.

WIP right now. Come back later for a more accurate description. 😇

Closes #1242 and #1238.

adamjohnson and others added 16 commits August 29, 2024 10:38
* feat(skip-link): href attr

* docs: skip-link no-anchor

* docs: update elements/rh-skip-link/demo/href-attribute.html

Co-authored-by: Adam Johnson <[email protected]>

---------

Co-authored-by: Adam Johnson <[email protected]>
* docs: remove RTI from sidenav

* fix(subnav): remove rti, add accessible-label attr

* fix(navigation-secondary): remove rti, add accessible-label attr

* chore: add changesets

* chore(subnav): update changeset

* chore(navigation-secondary): remove orphaned code

* docs: readd rti to sidenav taken care of in seperate docs pr

* docs(navigation-secondary): add accessible-label documentation

* docs(subnav): add accessible-label documentation

* chore(subnav): remove unused ifDefined import

* fix(navigation-secondary): remvoe preventDefault to allow tab to move to next tabstop

* fix(accordion): remove rti

* chore(navigation-secondary): update changeset

* chore(accordion): add changeet

* docs: update changesets

---------

Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
* fix(accordion): bold `rh-accordion-header` text

* fix(accordion): appease nanocss

---------

Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers <[email protected]>
* docs: theming

WIP, picking up where #1298 left off

* docs: pattern shortcode splits out css

* docs: pattern showcase element layout

* feat(card): header background theme props

* docs(card): asset patterns

* docs: pattern elements in typescript

* feat: themable surfaces

adds:
- `--rh-color-surface`
- `--rh-color-text-primary`
- `--rh-color-border-subtle`
- `--rh-color-border-strong`
- `--rh-color-border-interactive`

* docs(card): patterns with themable properties

* style(card): template

* docs(card): use themable properties

* fix(context): more themable props

* style: sort props

* feat: add --rh-color-theme-inverse

* chore: dev server layouts

* refactor(accordion): themable tokens

* chore: minor perf improvement for dev server

we still have a major problem of trace performance in
generator.htmlInject to work through

* feat: link colours

* docs: port uxdot-installation-tabs to typescript

* chore: wireit dependencies

* fix: update tokens stuff

* chore: deps

* chore: stylelint rules

* chore: temporarily patch tokens

* fix: port element styles

* fix(lib): port element styles

* docs: port tokens

* chore: update temporary patch

* style: lint css

* refactor(pagination): css

* chore: use tokens prerelease

* chore: use prerelease tokens

* docs: load rhds packages locally

* docs: handle theme tokens

* fix(context): use new tokens repo

* docs: load tokens from node_modules

* chore: tokens deps

* chore: update tokens prerelease

* chore: update prerelease tokens

* chore: update prerelease tokens

* style: config

* chore: update deps

* fix: lint css files

* chore: prerelease tokens

* fix(context-picker): dont swallow event

* fix(surface): make it a consumer

* docs: token theme swatches

* docs: tokens toc, alt-card for themes

* docs: ssr the lightness of token swatches

* docs: fix tokens page

* docs: fancier tags

* style: lint

* docs: demo dsd fix

* docs: icon color themables

* fix(surface): act as a pseudo-consumer

don't swallow the context-request event, but still propate your own
color values

* chore: docs server perf

* docs: icon,border,etc

* perf: remove unused decorator

* docs: tokens tables

* docs: typography tokens

* docs: bodge up token tables

these files are due for a serious refactor, but like... another day

* docs: themable tokens changeset

* docs: update theming prose

* test(surface): typescript nudges

* test(surface): fix tests

* test: console logs

* docs: tokens demo

* docs: fix tokens

* docs: nicer ids

* docs: refactor tokens table code

* fix(table): theme tokens

* docs: refactor token collection

* docs: refactor patterns, move links and scripts to head

* fix(subnav): use theme tokens

* fix(tile): use theme tokens

* docs: header, tile patterns

* fix(card): host display block

* docs: tile patterns and theming

* fix(card): surface tokens

* fix(tile): surface tokens

* fix(subnav): force color palette per guidelines

* fix(tile): use theme tokens

* feat(context-picker): pass an element ref

* docs(tile): theming patterns

* style: lint

* test: more sleep

* style: lint

* test: fix audio player tests

* docs: element pattern in color palette docs

* fix(cta): primary color

* fix(tabs): color context

* fix(context-demo): import specifier

* fix(tile): override surface colors

* docs: copy styles over

* docs: theming prose

* chore: simplify dev server config

* docs: footnotes

* docs: lint styles

* style: lint element css

* style: lint css comments

* docs: lint css

* fix(cta): context css

* chore: workaround in dev server for jspm limitation

* style: lint

* docs: tile lightdom

* docs: pullquote margins

* docs: bodge for audio-player layouts

see also #1264

* docs: fix import map?

* docs: remove url filter

* docs: theme collage

* docs: theming patterns

* docs: theming bordeaux example

* fix(card): color palettes and layouts

* docs(card): color palettes

* docs: more theming prose

* docs: more theming

* docs: bordeaux

* fix(code-block): styles

* fix(context-demo): context

* fix(blockquote): theme tokens

---------

Co-authored-by: Mark Caron <[email protected]>
* feat(code-block): wip client-side highlighting

* style: type linting

* chore: appease typescript for the sake of prism

see microsoft/TypeScript#20595

* docs: prism in 11ty

* fix(code-block): prism css module

* docs: eleventy prism import map

* docs: more import map shenanigans

* fix(code-block): highlighted line numbers

* docs: changeset

* docs(code-block): documentation for new attrs

* fix(code-block): disclosure aria

* fix(code-block): comment colors

* fix(code-block): tabindex

* fix: update elements/rh-code-block/rh-code-block.ts
@adamjohnson adamjohnson added the feature New feature or request label Sep 13, 2024
@adamjohnson adamjohnson self-assigned this Sep 13, 2024
Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: a8df42a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rhds/elements Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Sep 13, 2024

Deploy Preview for red-hat-design-system ready!

Name Link
🔨 Latest commit a8df42a
🔍 Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/66fffc042cb6e600089aeddd
😎 Deploy Preview https://deploy-preview-1865--red-hat-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bennypowers and others added 7 commits September 15, 2024 10:15
chore: lit-css ts transform

allow css nesting syntax in shadow css. it will be unrolled with postcss
* docs: picker targets

* fix(surface): set but don't inherit surface colors

* docs(card): bar pattern

* fix(accordion): style guidelines

* fix(alert): theme tokens

* fix(badge): context, styles

doesn't implement status tokens, yet

* fix(button): disabled on dark

does not add semantic tokens

* fix(blockquote): border style

* fix(health-index): border colors

* fix(tabs): context in panel

* docs(tabs): links in demo

* fix(icon): container size

* fix(badge): tests and backgrounds

* docs(card): links

* docs(theming): backgrounds

* docs(theming): stub for developer

* docs: deps

* docs: more prose

* feat(tag): context, compact, href, desaturated

Closes #1843

* docs: tag

* fix(tag): spacing

* fix(tag): spacing

* docs(tag): live samples

* test(tag): tests pass

* fix(blockquote): on

* docs(tag): slotted icon

* fix(blockquote): on

* fix(accordion): drop shadow

also refactors expand/collapse code

* fix(alert): use rh-icon
Copy link
Contributor

github-actions bot commented Sep 17, 2024

Size Change: +785 B (+0.43%)

Total Size: 184 kB

Filename Size Change
./elements/rh-dialog/rh-dialog.js 5.57 kB +785 B (+16.42%) ⚠️
ℹ️ View Unchanged
Filename Size
./elements.js 473 B
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-header.js 2.77 kB
./elements/rh-accordion/rh-accordion-panel.js 1.37 kB
./elements/rh-accordion/rh-accordion.js 3.17 kB
./elements/rh-alert/rh-alert.js 3.93 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-rate-stepper.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.53 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.43 kB
./elements/rh-audio-player/rh-audio-player.js 13.2 kB
./elements/rh-audio-player/rh-cue.js 2 kB
./elements/rh-audio-player/rh-transcript.js 2.75 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.9 kB
./elements/rh-back-to-top/rh-back-to-top.js 2.1 kB
./elements/rh-badge/rh-badge.js 1.1 kB
./elements/rh-blockquote/rh-blockquote.js 1.4 kB
./elements/rh-breadcrumb/rh-breadcrumb.js 1.5 kB
./elements/rh-button/rh-button.js 4.24 kB
./elements/rh-card/rh-card.js 3.48 kB
./elements/rh-code-block/prism.css.js 725 B
./elements/rh-code-block/prism.js 556 B
./elements/rh-code-block/rh-code-block.js 6.93 kB
./elements/rh-cta/rh-cta.js 3.89 kB
./elements/rh-dialog/yt-api.js 617 B
./elements/rh-footer/rh-footer-block.js 766 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.17 kB
./elements/rh-footer/rh-footer-social-link.js 964 B
./elements/rh-footer/rh-footer-universal.js 4.05 kB
./elements/rh-footer/rh-footer.js 5.01 kB
./elements/rh-health-index/rh-health-index.js 2.35 kB
./elements/rh-icon/rh-icon.js 2.36 kB
./elements/rh-icon/ssr.js 181 B
./elements/rh-menu/rh-menu.js 1.29 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.47 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.35 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.75 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 571 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.27 kB
./elements/rh-navigation-secondary/test/fixtures.js 769 B
./elements/rh-pagination/rh-pagination.js 5.46 kB
./elements/rh-site-status/rh-site-status.js 3.08 kB
./elements/rh-skip-link/rh-skip-link.js 1.24 kB
./elements/rh-spinner/rh-spinner.js 1.43 kB
./elements/rh-stat/rh-stat.js 2.2 kB
./elements/rh-subnav/rh-subnav.js 2.7 kB
./elements/rh-surface/rh-surface.js 1.11 kB
./elements/rh-surface/test/elements.js 423 B
./elements/rh-switch/rh-switch.js 2.92 kB
./elements/rh-table/rh-sort-button.js 1.49 kB
./elements/rh-table/rh-table.js 3.17 kB
./elements/rh-tabs/context.js 160 B
./elements/rh-tabs/rh-tab-panel.js 1.14 kB
./elements/rh-tabs/rh-tab.js 2.93 kB
./elements/rh-tabs/rh-tabs.js 3.68 kB
./elements/rh-tag/rh-tag.js 2.84 kB
./elements/rh-tile/rh-tile-group.js 1.81 kB
./elements/rh-tile/rh-tile.js 5.16 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.24 kB
./elements/rh-video-embed/rh-video-embed.js 4.6 kB
./lib/context/color/consumer.js 1.23 kB
./lib/context/color/controller.js 947 B
./lib/context/color/provider.js 2.08 kB
./lib/context/event.js 593 B
./lib/context/headings/consumer.js 722 B
./lib/context/headings/controller.js 1.12 kB
./lib/context/headings/provider.js 1.24 kB
./lib/DirController.js 565 B
./lib/elements/rh-context-demo/rh-context-demo.js 1.28 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.21 kB
./lib/environment.js 4.09 kB
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 849 B
./react/rh-accordion/rh-accordion-header.js 199 B
./react/rh-accordion/rh-accordion-panel.js 185 B
./react/rh-accordion/rh-accordion.js 215 B
./react/rh-alert/rh-alert.js 184 B
./react/rh-audio-player/rh-audio-player-about.js 191 B
./react/rh-audio-player/rh-audio-player-rate-stepper.js 213 B
./react/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 214 B
./react/rh-audio-player/rh-audio-player-subscribe.js 196 B
./react/rh-audio-player/rh-audio-player.js 183 B
./react/rh-audio-player/rh-cue.js 195 B
./react/rh-audio-player/rh-transcript.js 207 B
./react/rh-avatar/rh-avatar.js 173 B
./react/rh-back-to-top/rh-back-to-top.js 183 B
./react/rh-badge/rh-badge.js 174 B
./react/rh-blockquote/rh-blockquote.js 179 B
./react/rh-breadcrumb/rh-breadcrumb.js 179 B
./react/rh-button/rh-button.js 174 B
./react/rh-card/rh-card.js 172 B
./react/rh-code-block/rh-code-block.js 181 B
./react/rh-cta/rh-cta.js 170 B
./react/rh-dialog/rh-dialog.js 203 B
./react/rh-footer/rh-footer-block.js 184 B
./react/rh-footer/rh-footer-copyright.js 187 B
./react/rh-footer/rh-footer-links.js 185 B
./react/rh-footer/rh-footer-social-link.js 193 B
./react/rh-footer/rh-footer-universal.js 188 B
./react/rh-footer/rh-footer.js 174 B
./react/rh-health-index/rh-health-index.js 184 B
./react/rh-icon/rh-icon.js 202 B
./react/rh-menu/rh-menu.js 173 B
./react/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 217 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 205 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu.js 199 B
./react/rh-navigation-secondary/rh-navigation-secondary-overlay.js 201 B
./react/rh-navigation-secondary/rh-navigation-secondary.js 213 B
./react/rh-pagination/rh-pagination.js 178 B
./react/rh-site-status/rh-site-status.js 181 B
./react/rh-skip-link/rh-skip-link.js 181 B
./react/rh-spinner/rh-spinner.js 175 B
./react/rh-stat/rh-stat.js 171 B
./react/rh-subnav/rh-subnav.js 175 B
./react/rh-surface/rh-surface.js 175 B
./react/rh-switch/rh-switch.js 185 B
./react/rh-table/rh-sort-button.js 213 B
./react/rh-table/rh-table.js 174 B
./react/rh-tabs/rh-tab-panel.js 181 B
./react/rh-tabs/rh-tab.js 187 B
./react/rh-tabs/rh-tabs.js 174 B
./react/rh-tag/rh-tag.js 182 B
./react/rh-tile/rh-tile-group.js 183 B
./react/rh-tile/rh-tile.js 194 B
./react/rh-timestamp/rh-timestamp.js 176 B
./react/rh-tooltip/rh-tooltip.js 175 B
./react/rh-video-embed/rh-video-embed.js 227 B

compressed-size-action

adamjohnson and others added 5 commits September 17, 2024 14:09
* docs: icon design update

* docs: move icons to foundations

* docs(icon): feedback footer

* docs(icon): list styles

* docs(icon): fix a whoopsie

* docs(icon): layouts

* docs: copy assets

* docs(icons): fix permalink

* docs(icon): copy

* docs: icons feedback

* docs: change title of page and update section heading case

* docs: iconography order

* docs: align buttons

---------

Co-authored-by: marionnegp <[email protected]>
@bennypowers bennypowers removed this from the 2024/Q3 — Clefairy release milestone Sep 18, 2024
bennypowers and others added 13 commits September 18, 2024 11:00
* docs: simplify installation tabs

* docs: fix import map on installation page

* docs: jspm import maps

* docs: no pfe version

* docs: install tabs

* chore: patch prism-esm see if that lets build complete

see KonnorRogers/prism-esm#3

* docs: fail more gracefully when JSPM can't cope

* chore: lint nonsense

* docs: link to install info

* docs: installation tabs styles
* feat: adding automatic data-labels on col-scoped tables

* Update elements/rh-table/rh-table.ts

Co-authored-by: Adam Johnson <[email protected]>

* style(rh-table): linting error on extra curly brace

* Update elements/rh-table/rh-table.ts

Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>

* Update elements/rh-table/rh-table.ts

Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>

* feat(table): responsive attr, plus some style linting

* docs(table): updating prop definition

* refactor: prefer optional chaining to ternary

* refactor(table): use hammer operator and dataset to reduce footprint

* docs(table): adding responsive table instructions

* feat(table): making responsive layout default. Adjusting docs to reflect this.

* chore(table): adding changeset

* docs: update .changeset/khaki-rings-confess.md

* fix(table): mo

---------

Co-authored-by: Adam Johnson <[email protected]>
Co-authored-by: Benny Powers - עם ישראל חי! <[email protected]>
Co-authored-by: Benny Powers <[email protected]>
* fix(table): container query, css nesting, logical properties

* chore(table): add changeset

* chore(table): lint
* docs: add edit this page link

* docs: add edit this page to pattern template

* docs: edit this page trim leading dot

* fix(back-to-top): reconfigure pointer events to be on the trigger not host
Shift tab now works.

Shift tab works for a YouTube video dialog but normal tab does not trap focus.

Also, there's a glaring issue that, if an RHDS element is the last focusable element, it does not work. WIP!
@adamjohnson
Copy link
Collaborator Author

Current status

With a lot of help from @zeroedin, he and I were able to get Shift + Tab mostly working for <rh-dialog>. Check out the DP.

Setbacks

Just when we thought we had it cracked, we added an <rh-tabs> component to the body of the <rh-dialog> and found that it did not trap focus as intended if an <rh-*> element is the last focusable item in the dialog.

Possible solutions

We suspect that we'd have to add each focusable <rh-*> element to the focusable selectors list and possibly add delegatesFocus to each of those elements. We'd then have to maintain this focusable selectors list.

OR we could explore implementing a function like this that recursively traverses the Shadow DOM looking for focusable elements. Good to know we're not the first people to run into complexities with web components and focus traps.

OR we could add one of the off the shelf solutions as a dependency.

If anyone has strong feelings about any of these solutions, weigh in!

Video dialog + YouTube

Note that, on a video dialog with a YouTube embed, Shift + Tab works, but Tab does not trap focus. DP Video dialog demo

NB: Right now on uxdot for a dialog + YouTube embed, focus trapping works with Tab but not Shift + Tab. 🔁 uxdot production video demo

APG Guidelines vs. browser implementations

It's also worth noting that when a <dialog> is open, it does not allow other elements on the page to be focused. APG guidelines explicitly define the required keyboard behavior; however, the native <dialog> element does not follow this guidance. Accessibility practitioners are split as to which behavior is preferrable.

Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the surface need to be in the outside because of the overlay? Can we fix that with CSS and put the surface inside the dialog?

If we're using <dialog>, why do we still need kitty's focus trap javascript? Should we just call showModal by default?

Are we capturing and exposing the return value?

@adamjohnson
Copy link
Collaborator Author

Will look into moving <rh-surface> inside the dialog. 👍

If we're using <dialog>, why do we still need kitty's focus trap javascript? Should we just call showModal by default?

We are calling showModal() on line 370. APG keyboard interaction guidelines clearly state:

  • Tab:
    • Moves focus to the next tabbable element inside the dialog.
    • If focus is on the last tabbable element inside the dialog, moves focus to the first tabbable element inside the dialog.
  • Shift + Tab:
    • Moves focus to the previous tabbable element inside the dialog.
    • If focus is on the first tabbable element inside the dialog, moves focus to the last tabbable element inside the dialog.
  • Escape: Closes the dialog.

Again, browser's have implemented <dialog> to allow Tab access to things like other tabs and the URL bar instead of truly focus trapping the dialog.

Base automatically changed from staging/clefairy to main September 30, 2024 20:08
@adamjohnson adamjohnson changed the base branch from main to staging/cubone November 13, 2024 18:35
adamjohnson added a commit that referenced this pull request Dec 6, 2024
@adamjohnson
Copy link
Collaborator Author

Closing this PR due to merge conflicts. Continue this discussion in #2078.

@adamjohnson adamjohnson closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Status: Done ☑️
Development

Successfully merging this pull request may close these issues.

[feat] <rh-dialog>: switch to native <dialog> element [bug] <rh-dialog> keyboard focus issue
4 participants