From 65f418f8f191768b0fc0b92ffd2a681631594631 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 7 Dec 2023 10:19:38 -0800 Subject: [PATCH] Use floats to hide ActionBar items to address Android Chrome overflow issue (#2425) Co-authored-by: Jon Rohan --- .changeset/tough-pumas-guess.md | 5 + app/components/primer/alpha/action_bar.pcss | 14 +- .../primer/alpha/action_bar_element.ts | 171 ++++++++---------- test/system/alpha/action_bar_test.rb | 72 ++++++-- 4 files changed, 154 insertions(+), 108 deletions(-) create mode 100644 .changeset/tough-pumas-guess.md diff --git a/.changeset/tough-pumas-guess.md b/.changeset/tough-pumas-guess.md new file mode 100644 index 0000000000..87b09d328d --- /dev/null +++ b/.changeset/tough-pumas-guess.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Use floats to hide ActionBar items to address Android Chrome overflow issue diff --git a/app/components/primer/alpha/action_bar.pcss b/app/components/primer/alpha/action_bar.pcss index 86f11275ff..79104a2976 100644 --- a/app/components/primer/alpha/action_bar.pcss +++ b/app/components/primer/alpha/action_bar.pcss @@ -12,20 +12,18 @@ } .ActionBar-item-container { - display: flex; box-sizing: content-box; - align-items: center; - flex-shrink: 0; - flex-grow: 0; + overflow: hidden; + height: var(--control-medium-size); } .ActionBar-item { position: relative; - flex-shrink: 0; + float: left; } .ActionBar-more-menu { - flex-shrink: 0; + float: left; } .ActionBar--small { @@ -42,6 +40,10 @@ height: calc(var(--control-medium-size) / 2); margin: 0 var(--controlStack-medium-gap-condensed); border-left: var(--borderWidth-thin) solid var(--borderColor-muted); + float: left; + top: 50%; + bottom: 50%; + transform: translateY(-50%); } .ActionBar--small .ActionBar-divider { diff --git a/app/components/primer/alpha/action_bar_element.ts b/app/components/primer/alpha/action_bar_element.ts index affa2fd5e7..b6d518892f 100644 --- a/app/components/primer/alpha/action_bar_element.ts +++ b/app/components/primer/alpha/action_bar_element.ts @@ -1,5 +1,6 @@ import {controller, targets, target} from '@github/catalyst' import {focusZone, FocusKeys} from '@primer/behaviors' +import {ActionMenuElement} from './action_menu/action_menu_element' const instersectionObserver = new IntersectionObserver(entries => { for (const entry of entries) { @@ -14,25 +15,30 @@ const resizeObserver = new ResizeObserver(entries => { for (const entry of entries) { const action = entry.target if (action instanceof ActionBarElement) { - action.update(entry.contentRect) + action.update() } } }) +// These are definitely used, but eslint is dumb apparently + +// eslint-disable-next-line no-unused-vars +enum ItemType { + // eslint-disable-next-line no-unused-vars + Item, + // eslint-disable-next-line no-unused-vars + Divider, +} + @controller class ActionBarElement extends HTMLElement { @targets items: HTMLElement[] @target itemContainer: HTMLElement - @target moreMenu: HTMLElement + @target moreMenu: ActionMenuElement - #initialBarWidth: number - #previousBarWidth: number #focusZoneAbortController: AbortController | null = null connectedCallback() { - this.#previousBarWidth = this.offsetWidth ?? Infinity - this.#initialBarWidth = this.itemContainer.offsetWidth ?? Infinity - // Calculate the width of all the items before hiding anything for (const item of this.items) { const width = item.getBoundingClientRect().width @@ -44,10 +50,7 @@ class ActionBarElement extends HTMLElement { resizeObserver.observe(this) instersectionObserver.observe(this) - setTimeout(() => { - this.style.overflow = 'visible' - this.update() - }, 20) // Wait for the items to be rendered, making this really short to avoid a flash of unstyled content + requestAnimationFrame(() => this.update()) } disconnectedCallback() { @@ -65,114 +68,100 @@ class ActionBarElement extends HTMLElement { } } - update(rect: DOMRect = this.getBoundingClientRect()) { - // Only recalculate if the bar width changed - if (rect.width <= this.#previousBarWidth || this.#previousBarWidth === 0) { - this.#shrink() - } else if (rect.width > this.#previousBarWidth) { - this.#grow() - } - this.#previousBarWidth = rect.width + update() { + const firstItem = this.#firstItem + if (!firstItem) return + + const firstItemTop = firstItem.getBoundingClientRect().top + let previousItemType: ItemType | null = null + + this.#eachItem((item: HTMLElement, index: number, type: ItemType): boolean => { + const itemTop = item.getBoundingClientRect().top + + if (type === ItemType.Item) { + if (itemTop > firstItemTop) { + this.#hideItem(index) + + if (this.moreMenu.hidden) { + this.moreMenu.hidden = false + } + + if (previousItemType === ItemType.Divider) { + this.#hideItem(index - 1) + } + } else { + this.#showItem(index) + + if (index === this.items.length - 1) { + this.moreMenu.hidden = true + } + + if (previousItemType === ItemType.Divider) { + this.#showItem(index - 1) + } + } + } + + previousItemType = type + + return true + }) - if (rect.width <= this.#initialBarWidth) { - this.style.justifyContent = 'space-between' - } else { - this.style.justifyContent = 'flex-end' - } if (this.#focusZoneAbortController) { this.#focusZoneAbortController.abort() } + this.#focusZoneAbortController = focusZone(this, { bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd, focusOutBehavior: 'wrap', focusableElementFilter: element => { - return this.#isVisible(element) - } + const idx = this.items.indexOf(element.parentElement!) + const elementIsVisibleItem = idx > -1 && this.items[idx].style.visibility === 'visible' + const elementIsVisibleActionMenuInvoker = element === this.moreMenu.invokerElement && !this.moreMenu.hidden + return elementIsVisibleItem || elementIsVisibleActionMenuInvoker + }, }) } - #isVisible(element: HTMLElement): boolean { - // Safari doesn't support `checkVisibility` yet. - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - if (typeof element.checkVisibility === 'function') return element.checkVisibility() - - return Boolean(element.offsetParent || element.offsetWidth || element.offsetHeight) - } - - #itemGap(): number { - return parseInt(window.getComputedStyle(this.itemContainer)?.columnGap, 10) || 0 - } - - #availableSpace(): number { - // Get the offset of the item container from the container edge - return this.offsetWidth - this.itemContainer.offsetWidth - } + get #firstItem(): HTMLElement | null { + let foundItem = null - #menuSpace(): number { - if (this.moreMenu.hidden) { - return 0 - } - return this.moreMenu.offsetWidth + this.#itemGap() - } - - #shrink() { - if (this.items[0]!.hidden) { - return - } - - let index = this.items.length - 1 - for (const item of this.items.reverse()) { - if (!item.hidden && this.#availableSpace() < this.#menuSpace()) { - this.#hideItem(index) - } else if (this.#availableSpace() >= this.#menuSpace()) { - return + this.#eachItem((item: HTMLElement, _index: number, type: ItemType): boolean => { + if (type === ItemType.Item) { + foundItem = item + return false } - index-- - } - } - - #grow() { - // If last item is visible, there is no need to grow - if (!this.items[this.items.length - 1]!.hidden) { - return - } - let index = 0 - for (const item of this.items) { - if (item.hidden) { - const offsetWidth = Number(item.getAttribute('data-offset-width')) - if (this.#availableSpace() >= this.#menuSpace() + offsetWidth || index === this.items.length - 1) { - this.#showItem(index) - } else { - return - } - } - index++ - } + return true + }) - if (!this.items[this.items.length - 1]!.hidden) { - this.moreMenu.hidden = true - } + return foundItem } #showItem(index: number) { - this.items[index]!.hidden = false + this.items[index].style.setProperty('visibility', 'visible') this.#menuItems[index]!.hidden = true } #hideItem(index: number) { - this.items[index]!.hidden = true + this.items[index].style.setProperty('visibility', 'hidden') this.#menuItems[index]!.hidden = false - - if (this.moreMenu.hidden) { - this.moreMenu.hidden = false - } } get #menuItems(): NodeListOf { return this.moreMenu.querySelectorAll('[role="menu"] > li') } + + // eslint-disable-next-line no-unused-vars + #eachItem(callback: (item: HTMLElement, index: number, type: ItemType) => boolean): void { + for (let i = 0; i < this.items.length; i++) { + const item = this.items[i] + const type = item.classList.contains('ActionBar-divider') ? ItemType.Divider : ItemType.Item + if (!callback(item, i, type)) { + break + } + } + } } declare global { diff --git a/test/system/alpha/action_bar_test.rb b/test/system/alpha/action_bar_test.rb index 8e0661d27f..4cafaf94cb 100644 --- a/test/system/alpha/action_bar_test.rb +++ b/test/system/alpha/action_bar_test.rb @@ -3,20 +3,18 @@ require "system/test_case" class IntegrationAlphaActionBarTest < System::TestCase + include Primer::JsTestHelpers + def test_resizing_hides_items visit_preview(:default) - assert_selector("action-bar") do - assert_selector("[data-targets=\"action-bar.items\"]", count: 9) - refute_selector("[data-target=\"action-bar.moreMenu\"]") - end + assert_items_visible(count: 9) + refute_selector("[data-target=\"action-bar.moreMenu\"]") page.driver.browser.resize(width: 183, height: 350) - assert_selector("action-bar") do - assert_selector("[data-targets=\"action-bar.items\"]", count: 4) - assert_selector("[data-target=\"action-bar.moreMenu\"]") - end + assert_items_visible(count: 3) + assert_selector("[data-target=\"action-bar.moreMenu\"]") end def test_focus_set_on_first_item @@ -33,7 +31,7 @@ def test_focus_set_within_overflow_menu visit_preview(:default) page.driver.browser.resize(width: 145, height: 350) - assert_selector("[data-targets=\"action-bar.items\"]", count: 2) + assert_items_visible(count: 2) page.driver.browser.keyboard.type(:tab, :left) @@ -48,7 +46,7 @@ def test_escape_in_overflow_menu_sets_focus_back visit_preview(:default) page.driver.browser.resize(width: 145, height: 350) - assert_selector("[data-targets=\"action-bar.items\"]", count: 2) + assert_items_visible(count: 2) page.driver.browser.keyboard.type(:tab, :left) @@ -67,7 +65,7 @@ def test_click_outside_of_menu_sets_tabindex_back visit_preview(:default) page.driver.browser.resize(width: 145, height: 350) - assert_selector("[data-targets=\"action-bar.items\"]", count: 2) + assert_items_visible(count: 2) page.driver.browser.keyboard.type(:tab, :left) @@ -91,4 +89,56 @@ def test_arrow_left_loops_to_last_item # The last item "Attach" should be focused assert page.evaluate_script("document.activeElement.querySelector('svg.octicon-paperclip')") end + + def test_arrow_left_loops_to_last_item_after_resize + visit_preview(:default) + + page.driver.browser.resize(width: 183, height: 350) + assert_items_visible(count: 3) + + # Tab to first item and press left arrow to get to menu invoker, then last visible item + page.driver.browser.keyboard.type(:tab, :left, :left) + + # The ActionMenu invoker button should be focused + assert page.evaluate_script("document.activeElement.querySelector('svg.octicon-archive')") + end + + def test_dividers_are_never_right_most_item + # in other words, dividers are hidden when the item that immediately succeeds them is hidden + + visit_preview(:default) + page.driver.browser.resize(width: 290, height: 350) + assert_items_visible(count: 9) + + page.driver.browser.resize(width: 289, height: 350) + assert_items_visible(count: 7) + end + + def assert_items_visible(count:) + actual_count = nil + + 3.times do + actual_count = evaluate_multiline_script(<<~JS) + const items = document.querySelectorAll('[data-targets="action-bar.items"]'); + let count = 0; + + items.forEach((item) => { + if (item.style.visibility === "visible") count ++; + }); + + return count; + JS + + return true if count == actual_count + + # trigger component's #update method + page_width, page_height = page.driver.browser.viewport_size + page.driver.browser.resize(width: page_width + 1, height: page_height) + page.driver.browser.resize(width: page_width - 1, height: page_height) + + sleep 0.2 + end + + assert count == actual_count, "Expected #{count} items to be visible, found #{actual_count}" + end end