From 15d7aee663f9dae6885d01b927b78ae9239b7c55 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 30 Nov 2023 16:23:30 +0000 Subject: [PATCH 1/5] ensure scroll position does not change when opening actionmenu By using the `toggle` event we can ensure the popover is open and positioned (positioning happens during `beforetoggle`) before trying to focus the element. --- .../primer/alpha/action_menu/action_menu_element.ts | 6 +++++- previews/primer/alpha/action_menu_preview.rb | 7 +++++++ .../action_menu_preview/in_scroll_container.html.erb | 11 +++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 previews/primer/alpha/action_menu_preview/in_scroll_container.html.erb 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 ba4caffaaa..b3301df9de 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -107,6 +107,7 @@ export class ActionMenuElement extends HTMLElement { this.addEventListener('mouseover', this, {signal}) this.addEventListener('focusout', this, {signal}) this.addEventListener('mousedown', this, {signal}) + this.popoverElement?.addEventListener('toggle', this, {signal}) this.#setDynamicLabel() this.#updateInput() this.#softDisableItems() @@ -182,6 +183,10 @@ export class ActionMenuElement extends HTMLElement { const targetIsInvoker = this.invokerElement?.contains(event.target as HTMLElement) const eventIsActivation = this.#isActivation(event) + if (event.type === 'toggle' && (event as ToggleEvent).newState === 'open') { + this.#firstItem?.focus() + } + if (targetIsInvoker && event.type === 'mousedown') { this.#invokerBeingClicked = true return @@ -261,7 +266,6 @@ export class ActionMenuElement extends HTMLElement { this.#hide() } else { this.#show() - this.#firstItem?.focus() } } diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb index 84a0abbe89..ad9b0122c3 100644 --- a/previews/primer/alpha/action_menu_preview.rb +++ b/previews/primer/alpha/action_menu_preview.rb @@ -323,6 +323,13 @@ def opens_dialog(menu_id: "menu-1") }) end + # @label In Scoll container + # + def in_scroll_container + render_with_template() + end + + # @label Align end # def align_end(menu_id: "menu-1") diff --git a/previews/primer/alpha/action_menu_preview/in_scroll_container.html.erb b/previews/primer/alpha/action_menu_preview/in_scroll_container.html.erb new file mode 100644 index 0000000000..0f991715cd --- /dev/null +++ b/previews/primer/alpha/action_menu_preview/in_scroll_container.html.erb @@ -0,0 +1,11 @@ +
+ +
+ <%= render Primer::Alpha::ActionMenu.new(anchor_align: :end) do |c| %> + <% c.with_show_button { "Edit" } %> + <% c.with_item(tag: :button, type: "button", label: "Rename") %> + <% c.with_item(tag: :button, type: "button", scheme: :danger, label: "Remove") %> + <% end %> +
+ +
From f6a8725327f726eed8d717a633919bf0c4687a8e Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Fri, 1 Dec 2023 11:38:17 +0000 Subject: [PATCH 2/5] changeset --- .changeset/tall-emus-jump.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tall-emus-jump.md diff --git a/.changeset/tall-emus-jump.md b/.changeset/tall-emus-jump.md new file mode 100644 index 0000000000..a7a3bd2b8c --- /dev/null +++ b/.changeset/tall-emus-jump.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Ensure scroll position does not change when opening ActionMenus From 7c6a13cba25cc1ef24fc8ebcce30ce22049bd85c Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Thu, 14 Dec 2023 18:43:56 +0000 Subject: [PATCH 3/5] Adding test --- test/system/alpha/action_menu_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 2988e65088..74e3ab95a6 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -729,5 +729,17 @@ def test_fires_event_on_activation assert page.evaluate_script("window.activatedItemChecked") assert_equal "Fast forward", page.evaluate_script("window.activatedItemText") end + + def test_doesnt_scroll_when_opened + visit_preview(:in_scroll_container) + + page.evaluate_script("window.scrollTo(0,400)") + + assert_equal 400, page.evaluate_script("window.scrollY") + + click_on_invoker_button + + assert_equal 400, page.evaluate_script("window.scrollY") + end end end From aef8227ee7d2c7fa9ca67b28d8ca7da7ba02b79d Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Thu, 14 Dec 2023 21:39:40 +0000 Subject: [PATCH 4/5] fixing test --- test/system/alpha/action_menu_test.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 74e3ab95a6..eb2cb1dcac 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -732,13 +732,18 @@ def test_fires_event_on_activation def test_doesnt_scroll_when_opened visit_preview(:in_scroll_container) + focus_on_invoker_button + # Scroll to the invoker button page.evaluate_script("window.scrollTo(0,400)") + # Confirm that the page is scrolled assert_equal 400, page.evaluate_script("window.scrollY") - click_on_invoker_button + # Click on the button to open the menu + page.driver.browser.keyboard.type(:enter) + # Assert we're still scrolled assert_equal 400, page.evaluate_script("window.scrollY") end end From aec8e7955d34ff55574aba725b9205abf18378ba Mon Sep 17 00:00:00 2001 From: Jon Rohan Date: Fri, 15 Dec 2023 10:19:24 -0800 Subject: [PATCH 5/5] Update action_menu_test.rb --- test/system/alpha/action_menu_test.rb | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index eb2cb1dcac..2988e65088 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -729,22 +729,5 @@ def test_fires_event_on_activation assert page.evaluate_script("window.activatedItemChecked") assert_equal "Fast forward", page.evaluate_script("window.activatedItemText") end - - def test_doesnt_scroll_when_opened - visit_preview(:in_scroll_container) - focus_on_invoker_button - - # Scroll to the invoker button - page.evaluate_script("window.scrollTo(0,400)") - - # Confirm that the page is scrolled - assert_equal 400, page.evaluate_script("window.scrollY") - - # Click on the button to open the menu - page.driver.browser.keyboard.type(:enter) - - # Assert we're still scrolled - assert_equal 400, page.evaluate_script("window.scrollY") - end end end