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

ensure direct clicks to the dialog can only close the dialog #2553

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/strange-windows-occur.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/view-components": patch
---

Ensure only direct clicks to the dialog can close it
9 changes: 2 additions & 7 deletions app/components/primer/dialog_helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ function dialogInvokerButtonHandler(event: Event) {
// If the user is clicking a valid dialog trigger
let dialogId = button?.getAttribute('data-show-dialog-id')
if (dialogId) {
event.stopPropagation()
const dialog = document.getElementById(dialogId)
if (dialog instanceof HTMLDialogElement) {
dialog.showModal()
// A buttons default behaviour in some browsers it to send a pointer event
// If the behaviour is allowed through the dialog will be shown but then
// quickly hidden- as if it were never shown. This prevents that.
event.preventDefault()
event.stopPropagation()
}
}

Expand Down Expand Up @@ -72,11 +70,8 @@ export class DialogHelperElement extends HTMLElement {
handleEvent(event: MouseEvent) {
const target = event.target as HTMLElement
const dialog = this.dialog
if (!dialog?.open) return

// if the target is inside the dialog, but is not the dialog itself, leave
// the dialog open
if (target?.closest('dialog') === dialog && target !== dialog) return
// The click target _must_ be the dialog element itself, and not elements underneath or inside.
if (target !== dialog || !dialog?.open) return

const rect = dialog.getBoundingClientRect()
const clickWasInsideDialog =
Expand Down
15 changes: 15 additions & 0 deletions test/system/alpha/dialog_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,21 @@ def test_outside_menu_click_does_not_close_dialog
assert_selector "dialog[open]"
end

def test_click_events_can_be_added_to_invoker_buttons
# use this preview because it assigns a static ID to the invoker button
visit_preview(:with_header)

page.evaluate_script(<<~JS)
document.querySelector('#dialog-show-my-dialog').addEventListener('click', () => {
window.dialogInvokerClicked = true
})
JS

click_button("Show Dialog")

assert page.evaluate_script("window.dialogInvokerClicked"), "click event was not fired"
end

def test_dialog_inside_overlay_opens_when_clicked
visit_preview(:dialog_inside_overlay)

Expand Down
Loading