From 41020ed0bfa1fe4afb698534c516a43e50e83147 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 6 Dec 2023 20:53:09 +0000 Subject: [PATCH 1/4] make dialog region focusable via keyboard Co-authored-by: Erinna Chen --- app/components/primer/alpha/dialog.html.erb | 4 +- app/components/primer/alpha/dialog.rb | 6 ++- app/components/primer/primer.ts | 1 + app/components/primer/scrollable_region.ts | 48 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 app/components/primer/scrollable_region.ts diff --git a/app/components/primer/alpha/dialog.html.erb b/app/components/primer/alpha/dialog.html.erb index 8ebded10ce..dec087b0d4 100644 --- a/app/components/primer/alpha/dialog.html.erb +++ b/app/components/primer/alpha/dialog.html.erb @@ -5,7 +5,9 @@ <% if content.present? %> <%= content %> <% else %> - <%= body %> + + <%= body %> + <%= footer %> <% end %> <% end %> diff --git a/app/components/primer/alpha/dialog.rb b/app/components/primer/alpha/dialog.rb index e5efdc842f..65f7a27017 100644 --- a/app/components/primer/alpha/dialog.rb +++ b/app/components/primer/alpha/dialog.rb @@ -133,7 +133,7 @@ def initialize( @system_arguments, { aria: { disabled: true, - labelledby: "#{@id}-title", + labelledby: labelledby, describedby: "#{@id}-description" } } @@ -155,6 +155,10 @@ def before_render with_header unless header? with_body unless body? end + + def labelledby + "#{@id}-title" + end end end end diff --git a/app/components/primer/primer.ts b/app/components/primer/primer.ts index 0a8fc194af..a730255725 100644 --- a/app/components/primer/primer.ts +++ b/app/components/primer/primer.ts @@ -3,6 +3,7 @@ import './alpha/action_bar_element' import './alpha/dropdown' import './anchored_position' import './focus_group' +import './scrollable_region' import './alpha/image_crop' import './alpha/modal_dialog' import './beta/nav_list' diff --git a/app/components/primer/scrollable_region.ts b/app/components/primer/scrollable_region.ts new file mode 100644 index 0000000000..e18257ef3e --- /dev/null +++ b/app/components/primer/scrollable_region.ts @@ -0,0 +1,48 @@ +import {controller, attr} from '@github/catalyst' + +@controller +export class ScrollableRegionElement extends HTMLElement { + @attr hasOverflow = false + @attr labelledBy = '' + + observer: ResizeObserver + + connectedCallback() { + this.style.overflow = 'auto' + + this.observer = new ResizeObserver(entries => { + for (const entry of entries) { + this.hasOverflow = + entry.target.scrollHeight > entry.target.clientHeight || entry.target.scrollWidth > entry.target.clientWidth + } + }) + + this.observer.observe(this) + } + + disconnectedCallback() { + this.observer.disconnect() + } + + attributeChangedCallback(name: string) { + if (name === 'data-has-overflow') { + if (this.hasOverflow) { + this.setAttribute('aria-labelledby', this.labelledBy) + this.setAttribute('role', 'region') + this.setAttribute('tabindex', '0') + } else { + this.removeAttribute('aria-labelledby') + this.removeAttribute('role') + this.removeAttribute('tabindex') + } + } + } +} + +declare global { + interface Window { + ScrollableRegionElement: typeof ScrollableRegionElement + } +} + +window.ScrollableRegionElement = ScrollableRegionElement From 16626191c24a206a09f775df62d592100bdce219 Mon Sep 17 00:00:00 2001 From: David Strack Date: Tue, 12 Dec 2023 23:47:49 +0000 Subject: [PATCH 2/4] changeset --- .changeset/great-forks-work.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/great-forks-work.md diff --git a/.changeset/great-forks-work.md b/.changeset/great-forks-work.md new file mode 100644 index 0000000000..c82104df34 --- /dev/null +++ b/.changeset/great-forks-work.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Fix an accessibility issue where the Dialog body could not be reached via keyboard navigation From a3e1b1df82c3b1c77d5d1d24254e8268ee9a3507 Mon Sep 17 00:00:00 2001 From: David Strack Date: Fri, 15 Dec 2023 22:38:45 +0000 Subject: [PATCH 3/4] system test to assert dialog body gets focused --- test/system/alpha/dialog_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index 336c82d5a3..816068a7f2 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -67,5 +67,17 @@ def test_closes_dialog_that_is_not_top_level assert_selector "modal-dialog#dialog-one", visible: :hidden end + + def test_can_focus_dialog_body_with_keyboard + visit_preview(:long_text) + + click_button("Show Dialog") + + # tab twice to focus the dialog body + find("modal-dialog").send_keys(:tab).send_keys(:tab) + + # the scrollable-region element should now be focused after the two tab presses + assert_equal page.evaluate_script("document.activeElement.tagName"), "SCROLLABLE-REGION" + end end end From 183d574e4906889a0a77d13525953272951604cc Mon Sep 17 00:00:00 2001 From: David Strack Date: Mon, 18 Dec 2023 17:31:15 +0000 Subject: [PATCH 4/4] just load the preview in the system test --- test/system/alpha/dialog_test.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index 816068a7f2..fbafcb08a2 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -68,16 +68,11 @@ def test_closes_dialog_that_is_not_top_level assert_selector "modal-dialog#dialog-one", visible: :hidden end - def test_can_focus_dialog_body_with_keyboard + # We're just opening the dialog with a scrollable body + # so the Axe scan can ensure the scrollable region is compliant + def test_with_scrollable_body visit_preview(:long_text) - click_button("Show Dialog") - - # tab twice to focus the dialog body - find("modal-dialog").send_keys(:tab).send_keys(:tab) - - # the scrollable-region element should now be focused after the two tab presses - assert_equal page.evaluate_script("document.activeElement.tagName"), "SCROLLABLE-REGION" end end end