From 983e3a5fbf885c46d5bde0c749b0f9298ab2b53f Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Wed, 17 Jan 2024 11:30:18 +0000 Subject: [PATCH] Refactor Dialog to use `` internally (#2496) Co-authored-by: Jon Rohan --- .changeset/wise-coats-buy.md | 5 + .../alpha/action_menu/action_menu_element.ts | 8 + app/components/primer/alpha/dialog.html.erb | 4 +- app/components/primer/alpha/dialog.pcss | 194 +++++------------- app/components/primer/alpha/dialog.rb | 23 +-- app/components/primer/alpha/overlay.pcss | 2 +- app/components/primer/dialog_helper.ts | 77 +++++++ app/components/primer/primer.ts | 1 + ...with_dialog_moving_focus_to_input.html.erb | 5 +- test/components/primer/alpha/dialog_test.rb | 16 +- test/system/alpha/action_menu_test.rb | 8 +- test/system/alpha/dialog_test.rb | 14 +- test/system/alpha/tooltip_test.rb | 2 +- 13 files changed, 182 insertions(+), 177 deletions(-) create mode 100644 .changeset/wise-coats-buy.md create mode 100644 app/components/primer/dialog_helper.ts diff --git a/.changeset/wise-coats-buy.md b/.changeset/wise-coats-buy.md new file mode 100644 index 0000000000..d82c9d0b22 --- /dev/null +++ b/.changeset/wise-coats-buy.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': minor +--- + +Primer::Alpha::Dialog uses internally diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts index b3301df9de..2600f0ead7 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -279,7 +279,15 @@ export class ActionMenuElement extends HTMLElement { if (this.#isOpen()) { this.#hide() } + const activeElement = this.ownerDocument.activeElement + const lostFocus = this.ownerDocument.activeElement === this.ownerDocument.body + const focusInClosedMenu = this.contains(activeElement) + if (lostFocus || focusInClosedMenu) { + setTimeout(() => this.invokerElement?.focus(), 0) + } } + // a modal element will close all popovers + setTimeout(() => this.#show(), 0) dialog.addEventListener('close', handleDialogClose, {signal}) dialog.addEventListener('cancel', handleDialogClose, {signal}) } diff --git a/app/components/primer/alpha/dialog.html.erb b/app/components/primer/alpha/dialog.html.erb index dec087b0d4..ee7b921bac 100644 --- a/app/components/primer/alpha/dialog.html.erb +++ b/app/components/primer/alpha/dialog.html.erb @@ -1,5 +1,5 @@ <%= show_button %> -
+ <%= render Primer::BaseComponent.new(**@system_arguments) do %> <%= header %> <% if content.present? %> @@ -11,4 +11,4 @@ <%= footer %> <% end %> <% end %> -
+ diff --git a/app/components/primer/alpha/dialog.pcss b/app/components/primer/alpha/dialog.pcss index 83c9305605..3bce68e7ec 100644 --- a/app/components/primer/alpha/dialog.pcss +++ b/app/components/primer/alpha/dialog.pcss @@ -1,5 +1,9 @@ /* Overlay */ +dialog.Overlay:not([open]) { + display: none; +} + .Overlay--hidden { display: none !important; } @@ -13,12 +17,18 @@ .Overlay { display: flex; + inset: 0; + position: static; + margin: auto; + padding: 0; width: min(var(--overlay-width), 100vw - 2rem); min-width: 192px; max-height: min(calc(100vh - 2rem), var(--overlay-height)); white-space: normal; flex-direction: column; background-color: var(--overlay-bgColor); + color: var(--fgColor-default); + border: 0; border-radius: var(--borderRadius-large); box-shadow: var(--shadow-floating-small); opacity: 1; @@ -74,6 +84,23 @@ height: auto; } + &.Overlay--placement-left, &.Overlay--placement-right { + height: 100%; + max-height: unset; + } + + &.Overlay--placement-left { + inset: 0 auto 0 0; + border-top-left-radius: 0; + border-bottom-left-radius: 0; + } + + &.Overlay--placement-right { + inset: 0 0 0 auto; + border-top-right-radius: 0; + border-bottom-right-radius: 0; + } + /* start deprecate in favor of Alpha::Dialog */ &.Overlay--height-xsmall { height: min(192px, 100vh - 2rem); @@ -295,121 +322,50 @@ } } -@define-mixin Overlay-backdrop { - position: fixed; - top: 0; - right: 0; - bottom: 0; - left: 0; - z-index: 999; - display: flex; - background-color: var(--overlay-backdrop-bgColor, var(--color-neutral-muted)); -} - -@define-mixin Overlay-backdrop--transparent { - position: absolute; - z-index: 999; - background-color: transparent; -} - /* variants must be mixins so we can extend within a media query (@extend is not supported inside media queries) */ /* border-radius repeats within placement options to ensure the original radius is reset when two classes co-exist */ -/* center */ -@define-mixin Overlay-backdrop--center { - @mixin Overlay-backdrop; - - align-items: center; - justify-content: center; -} - -/* anchor */ -@define-mixin Overlay-backdrop--anchor { - @mixin Overlay-backdrop--transparent; +/* full width */ +.Overlay--full { + width: 100%; + max-width: 100vw; + height: 100%; + max-height: 100vh; + border-radius: unset !important; + flex-grow: 1; } -/* anchor side(s) */ -@define-mixin Overlay-backdrop--side $responsiveVariant { - @mixin Overlay-backdrop; - - /* default left */ - align-items: center; - justify-content: left; - - &.Overlay-backdrop--placement-left$(responsiveVariant) { - align-items: center; - justify-content: left; - - & > .Overlay$(responsiveVariant) { - height: 100vh; - max-height: unset; - border-radius: var(--borderRadius-large); - border-top-left-radius: 0; - border-bottom-left-radius: 0; +/* responsive variants */ - @media screen and (prefers-reduced-motion: no-preference) { - animation: 250ms cubic-bezier(0.33, 1, 0.68, 1) 0s 1 normal none running Overlay--motion-slideInRight; - } - } +/* --viewportRange-narrowLandscape */ +@media (max-width: 767px) { + .Overlay--placement-left-whenNarrow, .Overlay--placement-right-whenNarrow { + height: 100%; + max-height: 100vh; } - &.Overlay-backdrop--placement-right$(responsiveVariant) { - align-items: center; - justify-content: right; - - & > .Overlay$(responsiveVariant) { - height: 100vh; - max-height: unset; - border-radius: var(--borderRadius-large); - border-top-right-radius: 0; - border-bottom-right-radius: 0; - - @media screen and (prefers-reduced-motion: no-preference) { - animation: 250ms cubic-bezier(0.33, 1, 0.68, 1) 0s 1 normal none running Overlay--motion-slideInLeft; - } - } + .Overlay--placement-left-whenNarrow { + inset: 0 auto 0 0; + border-top-left-radius: 0; + border-bottom-left-radius: 0; } - &.Overlay-backdrop--placement-bottom$(responsiveVariant) { - align-items: end; - justify-content: center; - - & > .Overlay$(responsiveVariant) { - width: 100vw; - height: auto; - max-height: calc(100vh - 2rem); - border-radius: var(--borderRadius-large); - border-bottom-right-radius: 0; - border-bottom-left-radius: 0; - - @media screen and (prefers-reduced-motion: no-preference) { - animation: 250ms cubic-bezier(0.33, 1, 0.68, 1) 0s 1 normal none running Overlay--motion-slideUp; - } - } + .Overlay--placement-right-whenNarrow { + inset: 0 0 0 auto; + border-top-right-radius: 0; + border-bottom-right-radius: 0; } - &.Overlay-backdrop--placement-top$(responsiveVariant) { - align-items: start; - justify-content: center; - - & > .Overlay$(responsiveVariant) { - border-radius: var(--borderRadius-large); - border-top-left-radius: 0; - border-top-right-radius: 0; - - @media screen and (prefers-reduced-motion: no-preference) { - animation: 250ms cubic-bezier(0.33, 1, 0.68, 1) 0s 1 normal none running Overlay--motion-slideDown; - } - } + .Overlay--placement-bottom-whenNarrow { + width: 100%; + max-width: 100vw; + inset: auto 0 0; + border-bottom-left-radius: 0; + border-bottom-right-radius: 0; } -} -/* full width */ -@define-mixin Overlay-backdrop--full { - @mixin Overlay-backdrop; - - & .Overlay { + .Overlay--full-whenNarrow { width: 100%; max-width: 100vw; height: 100%; @@ -419,44 +375,6 @@ } } -/* Overlay variant classnames */ -.Overlay-backdrop--center { - @mixin Overlay-backdrop--center; -} - -.Overlay-backdrop--anchor { - @mixin Overlay-backdrop--anchor; -} - -.Overlay-backdrop--side { - @mixin Overlay-backdrop--side; -} - -.Overlay-backdrop--full { - @mixin Overlay-backdrop--full; -} - -/* responsive variants */ - -/* --viewportRange-narrowLandscape */ -@media (max-width: 767px) { - .Overlay-backdrop--center-whenNarrow { - @mixin Overlay-backdrop--center; - } - - .Overlay-backdrop--anchor-whenNarrow { - @mixin Overlay-backdrop--anchor; - } - - .Overlay-backdrop--side-whenNarrow { - @mixin Overlay-backdrop--side -whenNarrow; - } - - .Overlay-backdrop--full-whenNarrow { - @mixin Overlay-backdrop--full; - } -} - @keyframes Overlay--motion-slideDown { from { transform: translateY(-100%); diff --git a/app/components/primer/alpha/dialog.rb b/app/components/primer/alpha/dialog.rb index 65f7a27017..5ff5feed70 100644 --- a/app/components/primer/alpha/dialog.rb +++ b/app/components/primer/alpha/dialog.rb @@ -34,19 +34,19 @@ class Dialog < Primer::Component DEFAULT_POSITION = :center POSITION_MAPPINGS = { - DEFAULT_POSITION => "Overlay-backdrop--center", - :left => "Overlay-backdrop--side Overlay-backdrop--placement-left", - :right => "Overlay-backdrop--side Overlay-backdrop--placement-right" + DEFAULT_POSITION => "", + :left => "Overlay--placement-left", + :right => "Overlay--placement-right" }.freeze POSITION_OPTIONS = POSITION_MAPPINGS.keys DEFAULT_POSITION_NARROW = :inherit POSITION_NARROW_MAPPINGS = { DEFAULT_POSITION_NARROW => "", - :bottom => "Overlay-backdrop--side-whenNarrow Overlay-backdrop--placement-bottom-whenNarrow", - :fullscreen => "Overlay-backdrop--full-whenNarrow", - :left => "Overlay-backdrop--side-whenNarrow Overlay-backdrop--placement-left-whenNarrow", - :right => "Overlay-backdrop--side-whenNarrow Overlay-backdrop--placement-right-whenNarrow" + :bottom => "Overlay--placement-bottom-whenNarrow", + :fullscreen => "Overlay--full-whenNarrow", + :left => "Overlay--placement-left-whenNarrow", + :right => "Overlay--placement-right-whenNarrow" }.freeze POSITION_NARROW_OPTIONS = POSITION_NARROW_MAPPINGS.keys @@ -125,8 +125,7 @@ def initialize( @position_narrow = position_narrow @visually_hide_title = visually_hide_title - @system_arguments[:tag] = "modal-dialog" - @system_arguments[:role] = "dialog" + @system_arguments[:tag] = "dialog" @system_arguments[:id] = @id @system_arguments[:aria] = { modal: true } @system_arguments[:aria] = merge_aria( @@ -143,11 +142,9 @@ def initialize( "Overlay-whenNarrow", SIZE_MAPPINGS[fetch_or_fallback(SIZE_OPTIONS, @size, DEFAULT_SIZE)], "Overlay--motion-scaleFade", - system_arguments[:classes] - ) - @backdrop_classes = class_names( POSITION_MAPPINGS[fetch_or_fallback(POSITION_OPTIONS, @position, DEFAULT_POSITION)], - POSITION_NARROW_MAPPINGS[fetch_or_fallback(POSITION_NARROW_MAPPINGS, @position_narrow, DEFAULT_POSITION_NARROW)] + POSITION_NARROW_MAPPINGS[fetch_or_fallback(POSITION_NARROW_MAPPINGS, @position_narrow, DEFAULT_POSITION_NARROW)], + system_arguments[:classes] ) end diff --git a/app/components/primer/alpha/overlay.pcss b/app/components/primer/alpha/overlay.pcss index 945cadc52c..7e083eda32 100644 --- a/app/components/primer/alpha/overlay.pcss +++ b/app/components/primer/alpha/overlay.pcss @@ -15,7 +15,7 @@ anchored-position:not(.Overlay) { display: none } -anchored-position.not-anchored::backdrop { +anchored-position.not-anchored::backdrop, dialog::backdrop { background-color: var(--overlay-backdrop-bgColor, var(--color-neutral-muted)); } diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts new file mode 100644 index 0000000000..cba881cfb9 --- /dev/null +++ b/app/components/primer/dialog_helper.ts @@ -0,0 +1,77 @@ +function dialogInvokerButtonHandler(event: Event) { + const target = event.target as HTMLElement + const button = target?.closest('button') + + if (!button || button.hasAttribute('disabled') || button.getAttribute('aria-disabled') === 'true') return + + // 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() + } + } + + dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id') + if (dialogId) { + const dialog = document.getElementById(dialogId) + if (dialog instanceof HTMLDialogElement && dialog.open) { + dialog.close() + } + } +} + +export class DialogHelperElement extends HTMLElement { + get dialog() { + return this.querySelector('dialog') + } + + #abortController: AbortController | null = null + connectedCallback() { + const {signal} = (this.#abortController = new AbortController()) + document.addEventListener('click', dialogInvokerButtonHandler) + document.addEventListener('click', this, {signal}) + } + + disconnectedCallback() { + this.#abortController?.abort() + } + + handleEvent(event: MouseEvent) { + const target = event.target as HTMLElement + const dialog = this.dialog + if (!dialog?.open) return + if (target?.closest('dialog') !== dialog) return + + const rect = dialog.getBoundingClientRect() + const clickWasInsideDialog = + rect.top <= event.clientY && + event.clientY <= rect.top + rect.height && + rect.left <= event.clientX && + event.clientX <= rect.left + rect.width + + if (!clickWasInsideDialog) { + dialog.close() + } + } +} + +declare global { + interface Window { + DialogHelperElement: typeof DialogHelperElement + } + interface HTMLElementTagNameMap { + 'dialog-helper': DialogHelperElement + } +} + +if (!window.customElements.get('dialog-helper')) { + window.DialogHelperElement = DialogHelperElement + window.customElements.define('dialog-helper', DialogHelperElement) +} diff --git a/app/components/primer/primer.ts b/app/components/primer/primer.ts index a730255725..e0ba52eac3 100644 --- a/app/components/primer/primer.ts +++ b/app/components/primer/primer.ts @@ -2,6 +2,7 @@ import '@github/include-fragment-element' import './alpha/action_bar_element' import './alpha/dropdown' import './anchored_position' +import './dialog_helper' import './focus_group' import './scrollable_region' import './alpha/image_crop' diff --git a/previews/primer/alpha/tooltip_preview/tooltip_with_dialog_moving_focus_to_input.html.erb b/previews/primer/alpha/tooltip_preview/tooltip_with_dialog_moving_focus_to_input.html.erb index 82dc3a69d8..577b9f85c9 100644 --- a/previews/primer/alpha/tooltip_preview/tooltip_with_dialog_moving_focus_to_input.html.erb +++ b/previews/primer/alpha/tooltip_preview/tooltip_with_dialog_moving_focus_to_input.html.erb @@ -16,8 +16,7 @@ diff --git a/test/components/primer/alpha/dialog_test.rb b/test/components/primer/alpha/dialog_test.rb index 11a07406de..5d5e5b305b 100644 --- a/test/components/primer/alpha/dialog_test.rb +++ b/test/components/primer/alpha/dialog_test.rb @@ -12,7 +12,7 @@ def test_renders_title_and_body component.with_body { "Hello" } end - assert_selector("modal-dialog[role='dialog']") do + assert_selector("dialog") do assert_selector("h1", text: "Title") assert_selector(".Overlay-body", text: "Hello") end @@ -47,7 +47,7 @@ def test_renders_provided_id component.with_body { "content" } end - assert_selector("modal-dialog[id='my-id']") + assert_selector("dialog[id='my-id']") end def test_renders_random_id @@ -55,7 +55,7 @@ def test_renders_random_id component.with_body { "content" } end - assert_selector("modal-dialog[id^='dialog-']") + assert_selector("dialog[id^='dialog-']") end def test_renders_title_and_subtitle_with_describedby @@ -63,7 +63,7 @@ def test_renders_title_and_subtitle_with_describedby component.with_body { "content" } end - assert_selector("modal-dialog[id='my-dialog'][aria-labelledby='my-dialog-title'][aria-describedby='my-dialog-description']") do + assert_selector("dialog[id='my-dialog'][aria-labelledby='my-dialog-title'][aria-describedby='my-dialog-description']") do assert_selector("h1[id='my-dialog-title']", text: "Title") assert_selector("h2[id='my-dialog-description']", text: "Subtitle") end @@ -75,7 +75,7 @@ def test_renders_header component.with_header { "header" } end - assert_selector("modal-dialog") do + assert_selector("dialog") do assert_selector(".Overlay-header", text: "header") end end @@ -86,7 +86,7 @@ def test_renders_large_header component.with_header(variant: :large) { "header" } end - assert_selector("modal-dialog") do + assert_selector("dialog") do assert_selector(".Overlay-header.Overlay-header--large", text: "header") end end @@ -97,7 +97,7 @@ def test_renders_footer_without_divider_by_default component.with_footer { "footer" } end - assert_selector("modal-dialog") do + assert_selector("dialog") do assert_selector(".Overlay-footer:not(.Overlay-footer--divided)") end end @@ -108,7 +108,7 @@ def test_renders_footer_with_divider_if_show_divider_is_true component.with_footer(show_divider: true) { "footer" } end - assert_selector("modal-dialog") do + assert_selector("dialog") do assert_selector(".Overlay-footer.Overlay-footer--divided") end end diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 2988e65088..ccb14cda3e 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -316,7 +316,7 @@ def test_opens_dialog click_on_invoker_button click_on_second_item - assert_selector "modal-dialog[open]" + assert_selector "dialog[open]" # opening the dialog should close the menu refute_selector "action-menu ul li" @@ -330,7 +330,7 @@ def test_opens_dialog_on_keydown # open menu, arrow down to second item, "click" second item page.driver.browser.keyboard.type(:enter, :down, :enter) - assert_selector "modal-dialog#my-dialog" + assert_selector "dialog#my-dialog" end def test_opens_dialog_on_keydown_space @@ -341,7 +341,7 @@ def test_opens_dialog_on_keydown_space # open menu, arrow down to second item, "click" second item page.driver.browser.keyboard.type(:enter, :down, :space) - assert_selector "modal-dialog#my-dialog" + assert_selector "dialog#my-dialog" end def test_single_select_form_submission @@ -492,7 +492,7 @@ def test_deferred_dialog_opens click_on_invoker_button click_on_fourth_item - assert_selector "modal-dialog[open]" + assert_selector "dialog[open]", visible: :hidden # menu should close refute_selector "action-menu ul li" diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index fbafcb08a2..35c756b4d4 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -21,8 +21,8 @@ def test_modal_has_accessible_name click_button("Show Dialog") - assert_selector("modal-dialog[aria-labelledby]") - assert_equal(find("modal-dialog")["aria-labelledby"], find("h1")["id"]) + assert_selector("dialog[aria-labelledby]") + assert_equal(find("dialog")["aria-labelledby"], find("h1")["id"]) end def test_focuses_close_button @@ -47,12 +47,12 @@ def test_closes_top_level_dialog click_button("Show Dialog") click_on_nested_dialog_button - assert_equal(find("modal-dialog#dialog-two")["open"], true) + assert_equal(find("dialog#dialog-two")["open"], true) click_on_nested_dialog_close_button - assert_selector "modal-dialog#dialog-two", visible: :hidden - assert_selector "modal-dialog#dialog-one" + assert_selector "dialog#dialog-two", visible: :hidden + assert_selector "dialog#dialog-one" end def test_closes_dialog_that_is_not_top_level @@ -61,11 +61,11 @@ def test_closes_dialog_that_is_not_top_level click_button("Show Dialog") click_on_nested_dialog_button - assert_equal(find("modal-dialog#dialog-two")["open"], true) + assert_equal(find("dialog#dialog-two")["open"], true) click_on_initial_dialog_close_button - assert_selector "modal-dialog#dialog-one", visible: :hidden + assert_selector "dialog#dialog-one", visible: :hidden end # We're just opening the dialog with a scrollable body diff --git a/test/system/alpha/tooltip_test.rb b/test/system/alpha/tooltip_test.rb index 5f918fffbc..45b3fca1a9 100644 --- a/test/system/alpha/tooltip_test.rb +++ b/test/system/alpha/tooltip_test.rb @@ -132,7 +132,7 @@ def test_tooltip_hidden_after_focus_change find("button#dialog-show-my-dialog").click - find("modal-dialog#my-dialog button#yes-button").click + find("dialog#my-dialog button#yes-button").click assert_selector("tool-tip[for='dialog-show-my-dialog']", visible: :hidden) end