Upgrading to @oddbird/popover-polyfill 0.3.6 (#2415)

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/divider.rb

@@ -14,9 +14,6 @@ def initialize
             aria: {
               hidden: true
             },
-            data: {
-              targets: "action-bar.items"
-            },
             classes: "ActionBar-item ActionBar-divider"
           }
         end

app/components/primer/alpha/action_bar_element.ts

@@ -14,7 +14,7 @@ const resizeObserver = new ResizeObserver(entries => {
   for (const entry of entries) {
     const action = entry.target
     if (action instanceof ActionBarElement) {
-      action.update(entry.contentRect)
+      action.update()
     }
   }
 })
@@ -25,14 +25,9 @@ class ActionBarElement extends HTMLElement {
   @target itemContainer: HTMLElement
   @target moreMenu: HTMLElement
 
-  #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 +39,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,29 +57,38 @@ 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 firstItemTop = this.items[0].getBoundingClientRect().top
+
+    for (let i = 0; i < this.items.length; i++) {
+      const item = this.items[i]
+      const itemTop = item.getBoundingClientRect().top
+
+      if (itemTop > firstItemTop) {
+        this.#hideItem(i)
 
-    if (rect.width <= this.#initialBarWidth) {
-      this.style.justifyContent = 'space-between'
-    } else {
-      this.style.justifyContent = 'flex-end'
+        if (this.moreMenu.hidden) {
+          this.moreMenu.hidden = false
+        }
+      } else {
+        this.#showItem(i)
+
+        if (i === this.items.length - 1) {
+          this.moreMenu.hidden = true
+        }
+      }
     }
+
     if (this.#focusZoneAbortController) {
       this.#focusZoneAbortController.abort()
     }
+
     this.#focusZoneAbortController = focusZone(this, {
       bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd,
       focusOutBehavior: 'wrap',
       focusableElementFilter: element => {
         return this.#isVisible(element)
-      }
+      },
     })
   }
 
@@ -100,78 +101,18 @@ class ActionBarElement extends HTMLElement {
     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
-  }
-
-  #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
-      }
-      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++
-    }
-
-    if (!this.items[this.items.length - 1]!.hidden) {
-      this.moreMenu.hidden = true
-    }
-  }
-
   #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')
+    return this.moreMenu.querySelectorAll('[role="menu"] > li.ActionListItem')
   }
 }
 

package.json

@@ -51,7 +51,7 @@
     "@github/include-fragment-element": "^6.1.1",
     "@github/relative-time-element": "^4.0.0",
     "@github/tab-container-element": "^3.1.2",
-    "@oddbird/popover-polyfill": "^0.3.2",
+    "@oddbird/popover-polyfill": "^0.3.6",
     "@primer/behaviors": "^1.3.4"
   },
   "devDependencies": {
@@ -95,4 +95,4 @@
   },
   "prettier": "@github/prettier-config",
   "browserslist": "extends @github/browserslist-config"
-}
+}

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: 7)
+    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: 4)
+    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,30 @@ 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 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\"]');
+        const first_item_top = items[0].getBoundingClientRect().top;
+        let count = 0;
+
+        items.forEach((item) => {
+          if (item.getBoundingClientRect().top === first_item_top) {
+            count ++;
+          }
+        });
+
+        return count;
+      JS
+
+      return true if count == actual_count
+
+      sleep 0.1
+    end
+
+    assert count == actual_count, "Expected #{count} items to be visible, found #{actual_count}"
+  end
 end