Skip to content

Commit

Permalink
Make dialog region focusable via keyboard (primer#2428)
Browse files Browse the repository at this point in the history
Co-authored-by: Erinna Chen <erinnachen@users.noreply.github.com>
  • Loading branch information
strackoverflow and erinnachen authored Dec 18, 2023
1 parent 11ed01a commit 4cb9a57
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/great-forks-work.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/view-components': patch
---

Fix an accessibility issue where the Dialog body could not be reached via keyboard navigation
4 changes: 3 additions & 1 deletion app/components/primer/alpha/dialog.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
<% if content.present? %>
<%= content %>
<% else %>
<%= body %>
<scrollable-region data-labelled-by="<%= labelledby %>">
<%= body %>
</scrollable-region>
<%= footer %>
<% end %>
<% end %>
Expand Down
6 changes: 5 additions & 1 deletion app/components/primer/alpha/dialog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def initialize(
@system_arguments, {
aria: {
disabled: true,
labelledby: "#{@id}-title",
labelledby: labelledby,
describedby: "#{@id}-description"
}
}
Expand All @@ -154,6 +154,10 @@ def before_render
with_header unless header?
with_body unless body?
end

def labelledby
"#{@id}-title"
end
end
end
end
1 change: 1 addition & 0 deletions app/components/primer/primer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import './alpha/dropdown'
import './anchored_position'
import './dialog_helper'
import './focus_group'
import './scrollable_region'
import './alpha/image_crop'
import './beta/nav_list'
import './beta/nav_list_group_element'
Expand Down
48 changes: 48 additions & 0 deletions app/components/primer/scrollable_region.ts
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions test/system/alpha/dialog_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,12 @@ def test_closes_dialog_that_is_not_top_level

assert_selector "dialog#dialog-one", visible: :hidden
end

# 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")
end
end
end

0 comments on commit 4cb9a57

Please sign in to comment.