Use floats to hide ActionBar items to address Android Chrome overflow…

… issue (#2425)

Co-authored-by: Jon Rohan <rohan@github.com>

.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

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 {

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<HTMLElement> {
     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 {

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