diff --git a/.changeset/strange-rings-confess.md b/.changeset/strange-rings-confess.md new file mode 100644 index 0000000000..c834c055c1 --- /dev/null +++ b/.changeset/strange-rings-confess.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': minor +--- + +[ActionMenu] Don't allow previously hidden items to be checkable; add JavaScript API diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index 66da459c7e..5e5d08d602 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -128,7 +128,7 @@ class Item < Primer::Component # @private renders_one :private_content - attr_reader :id, :list, :href, :active, :disabled, :parent + attr_reader :id, :item_id, :list, :href, :active, :disabled, :parent # Whether or not this item is active. # @@ -143,6 +143,7 @@ class Item < Primer::Component # @param list [Primer::Alpha::ActionList] The list that contains this item. Used internally. # @param parent [Primer::Alpha::ActionList::Item] This item's parent item. `nil` if this item is at the root. Used internally. # @param label [String] Item label. If no label is provided, content is used. + # @param item_id [String] An ID that will be attached to the item's `
  • ` element as `data-item-id` for distinguishing between items, perhaps in JavaScript. # @param label_classes [String] CSS classes that will be added to the label. # @param label_arguments [Hash] <%= link_to_system_arguments_docs %> used to construct the label. # @param content_arguments [Hash] <%= link_to_system_arguments_docs %> used to construct the item's anchor or button tag. @@ -161,6 +162,7 @@ class Item < Primer::Component def initialize( list:, label: nil, + item_id: nil, label_classes: nil, label_arguments: {}, content_arguments: {}, @@ -181,6 +183,7 @@ def initialize( @list = list @parent = parent @label = label + @item_id = item_id @href = href || content_arguments[:href] @truncate_label = truncate_label @disabled = disabled @@ -206,6 +209,15 @@ def initialize( @system_arguments[:data] ||= {} @system_arguments[:data][:targets] = "#{list_class.custom_element_name}.items" + @system_arguments[:data] = merge_data( + @system_arguments, { + data: { + targets: "#{list_class.custom_element_name}.items", + **(@item_id ? { item_id: @item_id } : {}) + } + } + ) + @label_arguments = { **label_arguments, classes: class_names( diff --git a/app/components/primer/alpha/action_menu.rb b/app/components/primer/alpha/action_menu.rb index 6704aef5a6..8b01d622d5 100644 --- a/app/components/primer/alpha/action_menu.rb +++ b/app/components/primer/alpha/action_menu.rb @@ -128,6 +128,39 @@ module Alpha # # Additional information around the keyboard functionality and implementation can be found on the # [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.2/#menu). + # + # ### JavaScript API + # + # `ActionList`s render an `` custom element that exposes behavior to the client. For all these methods, + # `itemId` refers to the value of the `item_id:` argument (see below) that is used to populate the `data-item-id` HTML + # attribute. + # + # #### Query methods + # + # * `getItemById(itemId: string): Element`: Returns the item's HTML `
  • ` element. The return value can be passed as the `item` argument to the other methods listed below. + # * `isItemChecked(item: Element): boolean`: Returns `true` if the item is checked, `false` otherwise. + # * `isItemHidden(item: Element): boolean`: Returns `true` if the item is hidden, `false` otherwise. + # * `isItemDisabled(item: Element): boolean`: Returns `true` if the item is disabled, `false` otherwise. + # + # #### State methods + # + # * `showItem(item: Element)`: Shows the item, i.e. makes it visible. + # * `hideItem(item: Element)`: Hides the item, i.e. makes it invisible. + # * `enableItem(item: Element)`: Enables the item, i.e. makes it clickable by the mouse and keyboard. + # * `disableItem(item: Element)`: Disables the item, i.e. makes it unclickable by the mouse and keyboard. + # * `checkItem(item: Element)`: Checks the item. Only has an effect in single- and multi-select modes. + # * `uncheckItem(item: Element)`: Unchecks the item. Only has an effect in multi-select mode, since items cannot be unchecked in single-select mode. + # + # #### Events + # + # The `` element fires an `itemActivated` event whenever an item is activated (eg. clicked) via the mouse or keyboard. + # + # ```typescript + # document.querySelector("action-menu").addEventListener("itemActivated", (event: ItemActivatedEvent) => { + # event.item // Element: the
  • item that was activated + # event.checked // boolean: whether or not the result of the activation checked the item + # }) + # ``` class ActionMenu < Primer::Component status :alpha 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 887028331c..ba4caffaaa 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -12,6 +12,17 @@ type SelectedItem = { const validSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[role="menuitemradio"]'] const menuItemSelectors = validSelectors.map(selector => `:not([hidden]) > ${selector}`) +export type ItemActivatedEvent = { + item: Element + checked: boolean +} + +declare global { + interface HTMLElementEventMap { + itemActivated: CustomEvent + } +} + @controller export class ActionMenuElement extends HTMLElement { @target @@ -82,7 +93,7 @@ export class ActionMenuElement extends HTMLElement { results.push({ label: labelEl?.textContent, value: selectedItem?.getAttribute('data-value'), - element: selectedItem + element: selectedItem, }) } @@ -102,35 +113,39 @@ export class ActionMenuElement extends HTMLElement { if (this.includeFragment) { this.includeFragment.addEventListener('include-fragment-replaced', this, { - signal + signal, }) } } + disconnectedCallback() { + this.#abortController.abort() + } + #softDisableItems() { const {signal} = this.#abortController - for (const item of this.#items) { + for (const item of this.querySelectorAll(validSelectors.join(','))) { item.addEventListener('click', this.#potentiallyDisallowActivation.bind(this), {signal}) item.addEventListener('keydown', this.#potentiallyDisallowActivation.bind(this), {signal}) } } - #potentiallyDisallowActivation(event: Event) { - if (!this.#isActivation(event)) return + // returns true if activation was prevented + #potentiallyDisallowActivation(event: Event): boolean { + if (!this.#isActivation(event)) return false const item = (event.target as HTMLElement).closest(menuItemSelectors.join(',')) - if (!item) return + if (!item) return false if (item.getAttribute('aria-disabled')) { event.preventDefault() event.stopPropagation() event.stopImmediatePropagation() + return true } - } - disconnectedCallback() { - this.#abortController.abort() + return false } #isKeyboardActivation(event: Event): boolean { @@ -202,6 +217,8 @@ export class ActionMenuElement extends HTMLElement { const targetIsItem = item !== null if (targetIsItem && eventIsActivation) { + if (this.#potentiallyDisallowActivation(event)) return + const dialogInvoker = item.closest('[data-show-dialog-id]') if (dialogInvoker) { @@ -214,7 +231,7 @@ export class ActionMenuElement extends HTMLElement { } this.#activateItem(event, item) - this.#handleItemActivated(event, item) + this.#handleItemActivated(item) // Pressing the space key on a button or link will cause the page to scroll unless preventDefault() // is called. While calling preventDefault() appears to have no effect on link navigation, it skips @@ -263,7 +280,7 @@ export class ActionMenuElement extends HTMLElement { dialog.addEventListener('cancel', handleDialogClose, {signal}) } - #handleItemActivated(event: Event, item: Element) { + #handleItemActivated(item: Element) { // Hide popover after current event loop to prevent changes in focus from // altering the target of the event. Not doing this specifically affects // tags. It causes the event to be sent to the currently focused element @@ -304,6 +321,11 @@ export class ActionMenuElement extends HTMLElement { } this.#updateInput() + this.dispatchEvent( + new CustomEvent('itemActivated', { + detail: {item: item.parentElement, checked: this.isItemChecked(item.parentElement)}, + }), + ) } #activateItem(event: Event, item: Element) { @@ -410,9 +432,85 @@ export class ActionMenuElement extends HTMLElement { return this.querySelector(menuItemSelectors.join(',')) } - get #items(): HTMLElement[] { + get items(): HTMLElement[] { return Array.from(this.querySelectorAll(menuItemSelectors.join(','))) } + + getItemById(itemId: string): HTMLElement | null { + return this.querySelector(`li[data-item-id="${itemId}"`) + } + + isItemDisabled(item: Element | null): boolean { + if (item) { + return item.classList.contains('ActionListItem--disabled') + } else { + return false + } + } + + disableItem(item: Element | null) { + if (item) { + item.classList.add('ActionListItem--disabled') + item.querySelector('.ActionListContent')!.setAttribute('aria-disabled', 'true') + } + } + + enableItem(item: Element | null) { + if (item) { + item.classList.remove('ActionListItem--disabled') + item.querySelector('.ActionListContent')!.removeAttribute('aria-disabled') + } + } + + isItemHidden(item: Element | null): boolean { + if (item) { + return item.hasAttribute('hidden') + } else { + return false + } + } + + hideItem(item: Element | null) { + if (item) { + item.setAttribute('hidden', 'hidden') + } + } + + showItem(item: Element | null) { + if (item) { + item.removeAttribute('hidden') + } + } + + isItemChecked(item: Element | null) { + if (item) { + return item.querySelector('.ActionListContent')!.getAttribute('aria-checked') === 'true' + } else { + return false + } + } + + checkItem(item: Element | null) { + if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { + const itemContent = item.querySelector('.ActionListContent')! + const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' + + if (!ariaChecked) { + this.#handleItemActivated(itemContent) + } + } + } + + uncheckItem(item: Element | null) { + if (item && (this.selectVariant === 'single' || this.selectVariant === 'multiple')) { + const itemContent = item.querySelector('.ActionListContent')! + const ariaChecked = itemContent.getAttribute('aria-checked') === 'true' + + if (ariaChecked) { + this.#handleItemActivated(itemContent) + } + } + } } if (!window.customElements.get('action-menu')) { diff --git a/app/components/primer/alpha/action_menu/list.rb b/app/components/primer/alpha/action_menu/list.rb index d37bef1377..0749a815e6 100644 --- a/app/components/primer/alpha/action_menu/list.rb +++ b/app/components/primer/alpha/action_menu/list.rb @@ -11,7 +11,7 @@ class List < Primer::Component attr_reader :items - # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionMenu::List) %> + # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList) %> def initialize(**system_arguments) @items = [] @has_group = false diff --git a/package-lock.json b/package-lock.json index 82f3cae5a9..d2eba4b55b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,7 +42,7 @@ "eslint": "^8.23.1", "eslint-plugin-custom-elements": "^0.0.6", "eslint-plugin-github": "^4.9.2", - "eslint-plugin-prettier": "^4.2.1", + "eslint-plugin-prettier": "^5.0.0", "markdownlint-cli2": "^0.10.0", "mocha": "^10.0.0", "playwright": "^1.35.1", @@ -4913,35 +4913,6 @@ "eslint": "^8.0.1" } }, - "node_modules/eslint-plugin-github/node_modules/eslint-plugin-prettier": { - "version": "5.0.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", - "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", - "dev": true, - "dependencies": { - "prettier-linter-helpers": "^1.0.0", - "synckit": "^0.8.5" - }, - "engines": { - "node": "^14.18.0 || >=16.0.0" - }, - "funding": { - "url": "https://opencollective.com/prettier" - }, - "peerDependencies": { - "@types/eslint": ">=8.0.0", - "eslint": ">=8.0.0", - "prettier": ">=3.0.0" - }, - "peerDependenciesMeta": { - "@types/eslint": { - "optional": true - }, - "eslint-config-prettier": { - "optional": true - } - } - }, "node_modules/eslint-plugin-i18n-text": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/eslint-plugin-i18n-text/-/eslint-plugin-i18n-text-1.0.1.tgz", @@ -5059,21 +5030,29 @@ } }, "node_modules/eslint-plugin-prettier": { - "version": "4.2.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.2.1.tgz", - "integrity": "sha512-f/0rXLXUt0oFYs8ra4w49wYZBG5GKZpAYsJSm6rnYL5uVDjd+zowwMwVZHnAjf4edNrKpCDYfXDgmRE/Ak7QyQ==", + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", + "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", "dev": true, "dependencies": { - "prettier-linter-helpers": "^1.0.0" + "prettier-linter-helpers": "^1.0.0", + "synckit": "^0.8.5" }, "engines": { - "node": ">=12.0.0" + "node": "^14.18.0 || >=16.0.0" + }, + "funding": { + "url": "https://opencollective.com/prettier" }, "peerDependencies": { - "eslint": ">=7.28.0", - "prettier": ">=2.0.0" + "@types/eslint": ">=8.0.0", + "eslint": ">=8.0.0", + "prettier": ">=3.0.0" }, "peerDependenciesMeta": { + "@types/eslint": { + "optional": true + }, "eslint-config-prettier": { "optional": true } @@ -15187,18 +15166,6 @@ "jsx-ast-utils": "^3.3.2", "prettier": "^3.0.0", "svg-element-attributes": "^1.3.1" - }, - "dependencies": { - "eslint-plugin-prettier": { - "version": "5.0.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", - "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", - "dev": true, - "requires": { - "prettier-linter-helpers": "^1.0.0", - "synckit": "^0.8.5" - } - } } }, "eslint-plugin-i18n-text": { @@ -15296,12 +15263,13 @@ "dev": true }, "eslint-plugin-prettier": { - "version": "4.2.1", - "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.2.1.tgz", - "integrity": "sha512-f/0rXLXUt0oFYs8ra4w49wYZBG5GKZpAYsJSm6rnYL5uVDjd+zowwMwVZHnAjf4edNrKpCDYfXDgmRE/Ak7QyQ==", + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-5.0.1.tgz", + "integrity": "sha512-m3u5RnR56asrwV/lDC4GHorlW75DsFfmUcjfCYylTUs85dBRnB7VM6xG8eCMJdeDRnppzmxZVf1GEPJvl1JmNg==", "dev": true, "requires": { - "prettier-linter-helpers": "^1.0.0" + "prettier-linter-helpers": "^1.0.0", + "synckit": "^0.8.5" } }, "eslint-rule-documentation": { diff --git a/package.json b/package.json index 3362a32d02..9443853233 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "eslint": "^8.23.1", "eslint-plugin-custom-elements": "^0.0.6", "eslint-plugin-github": "^4.9.2", - "eslint-plugin-prettier": "^4.2.1", + "eslint-plugin-prettier": "^5.0.0", "markdownlint-cli2": "^0.10.0", "mocha": "^10.0.0", "playwright": "^1.35.1", diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb index 025cd656bd..84a0abbe89 100644 --- a/previews/primer/alpha/action_menu_preview.rb +++ b/previews/primer/alpha/action_menu_preview.rb @@ -148,10 +148,12 @@ def with_icon_button def single_select render(Primer::Alpha::ActionMenu.new(select_variant: :single)) do |menu| menu.with_show_button { "Menu" } - menu.with_item(label: "Fast forward") - menu.with_item(label: "Recursive") - menu.with_item(label: "Ours") - menu.with_item(label: "Resolve") + menu.with_item(label: "Fast forward", item_id: :fast_forward) + menu.with_item(label: "Recursive", item_id: :recursive) + menu.with_item(label: "Ours", item_id: :ours) + menu.with_item(label: "Resolve", item_id: :resolve) + menu.with_item(label: "Disabled", disabled: true, item_id: :disabled) + menu.with_item(label: "Hidden", hidden: true, disabled: true, item_id: :hidden) end end @@ -166,7 +168,8 @@ def multiple_select username: "langermank", full_name: "Katie Langerman", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :katie ) menu.with_avatar_item( @@ -174,7 +177,8 @@ def multiple_select username: "jonrohan", full_name: "Jon Rohan", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :jon ) menu.with_avatar_item( @@ -182,7 +186,8 @@ def multiple_select username: "broccolinisoup", full_name: "Armağan Ersöz", full_name_scheme: :inline, - avatar_arguments: { shape: :square } + avatar_arguments: { shape: :square }, + item_id: :armagan ) end end diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 8c0d32efa0..c6c366f930 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -17,6 +17,10 @@ def click_on_item(idx) items[idx - 1].click end + def click_on_item_by_id(id) + find("li[data-item-id='#{id}']").click + end + def click_on_first_item click_on_item(1) end @@ -46,6 +50,12 @@ def assert_no_alert(message = nil, &block) # expected behavior end + def evaluate_multiline_script(script) + page.evaluate_script(<<~JS) + (() => { #{script} })() + JS + end + ########## TESTS ############ def test_dynamic_labels @@ -599,5 +609,130 @@ def test_closes_menu_when_open_on_invoker_click click_on_invoker_button refute_selector "action-menu ul li" end + + def test_unhidden_disabled_item_cannot_be_checked + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=hidden]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.showItem(menu.getItemById('hidden')) + JS + + assert_selector "li[data-item-id=hidden]" + + click_on_item_by_id("hidden") + refute_selector "li [aria-checked=true]" + end + + def test_hide_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + assert_selector "li[data-item-id=recursive]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.hideItem(menu.getItemById('recursive')); + JS + + refute_selector "li[data-item-id=recursive]" + end + + def test_show_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=hidden]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.showItem(menu.getItemById('hidden')) + JS + + assert_selector "li[data-item-id=hidden]" + end + + def test_disable_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=resolve] [aria-disabled=true]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.disableItem(menu.getItemById('resolve')) + JS + + assert_selector "li[data-item-id=resolve] [aria-disabled=true]" + end + + def test_enable_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + assert_selector "li[data-item-id=disabled] [aria-disabled=true]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.enableItem(menu.getItemById('disabled')) + JS + + refute_selector "li[data-item-id=disabled] [aria-disabled=true]" + end + + def test_check_item_via_js_api + visit_preview(:single_select) + + click_on_invoker_button + refute_selector "li[data-item-id=recursive] [aria-checked=true]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.checkItem(menu.getItemById('recursive')) + JS + + click_on_invoker_button + assert_selector "li[data-item-id=recursive] [aria-checked=true]" + end + + def test_uncheck_item_via_js_api + # use multiple_select preview here because single-select menus do not allow unchecking checked items + visit_preview(:multiple_select) + + click_on_invoker_button + click_on_item_by_id("jon") + + assert_selector "li[data-item-id=jon] [aria-checked=true]" + + evaluate_multiline_script(<<~JS) + const menu = document.querySelector('action-menu') + menu.uncheckItem(menu.getItemById('jon')) + JS + + refute_selector "li[data-item-id=jon] [aria-checked=true]" + end + + def test_fires_event_on_activation + visit_preview(:single_select) + + evaluate_multiline_script(<<~JS) + window.activatedItemText = null + window.activatedItemChecked = false + + document.querySelector('action-menu').addEventListener('itemActivated', (event) => { + window.activatedItemText = event.detail.item.innerText + window.activatedItemChecked = event.detail.checked + }) + JS + + click_on_invoker_button + click_on_first_item + + assert page.evaluate_script("window.activatedItemChecked") + assert_equal "Fast forward", page.evaluate_script("window.activatedItemText") + end end end