Bugfix FXIOS-10500 - [Toolbar Redesign] Menu not displayed after tapp…

…ing on button (#23312)

BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift

@@ -35,6 +35,10 @@ public class BrowserAddressToolbar: UIView,
     private var theme: Theme?
     private var droppableUrl: URL?
 
+    /// A cache of `ToolbarButton` instances keyed by their accessibility identifier (`a11yId`).
+    /// This improves performance by reusing buttons instead of creating new instances.
+    private(set) var cachedButtonReferences = [String: ToolbarButton]()
+
     private lazy var toolbarContainerView: UIView = .build()
     private lazy var navigationActionStack: UIStackView = .build()
 
@@ -232,6 +236,7 @@ public class BrowserAddressToolbar: UIView,
         setNeedsLayout()
     }
 
+    // MARK: - Toolbar Actions and Layout Updates
     internal func updateActions(state: AddressToolbarState) {
         // Browser actions
         updateActionStack(stackView: browserActionStack, toolbarElements: state.browserActions)
@@ -256,10 +261,28 @@ public class BrowserAddressToolbar: UIView,
         widthAnchor.priority = .defaultHigh
     }
 
+    /// Retrieves a `ToolbarButton` for the given `ToolbarElement`.
+    /// If a cached button exists for the element's accessibility identifier, it returns the cached button.
+    /// Otherwise, it creates a new button, caches it, and then returns it.
+    /// - Parameter toolbarElement: The `ToolbarElement` for which to retrieve the button.
+    /// - Returns: A `ToolbarButton` instance configured for the given `ToolbarElement`.
+    func getToolbarButton(for toolbarElement: ToolbarElement) -> ToolbarButton {
+        let button: ToolbarButton
+        if let cachedButton = cachedButtonReferences[toolbarElement.a11yId] {
+            button = cachedButton
+        } else {
+            button = toolbarElement.numberOfTabs != nil ? TabNumberButton() : ToolbarButton()
+            cachedButtonReferences[toolbarElement.a11yId] = button
+        }
+
+        return button
+    }
+
     private func updateActionStack(stackView: UIStackView, toolbarElements: [ToolbarElement]) {
         stackView.removeAllArrangedViews()
+
         toolbarElements.forEach { toolbarElement in
-            let button = toolbarElement.numberOfTabs != nil ? TabNumberButton() : ToolbarButton()
+            let button = getToolbarButton(for: toolbarElement)
             button.configure(element: toolbarElement)
             stackView.addArrangedSubview(button)
 

BrowserKit/Sources/ToolbarKit/ToolbarButton.swift

@@ -59,6 +59,12 @@ class ToolbarButton: UIButton, ThemeApplicable {
         accessibilityIdentifier = element.a11yId
         accessibilityLabel = element.a11yLabel
         accessibilityHint = element.a11yHint
+        // Remove all existing actions for .touchUpInside before adding the new one
+        // This ensures that we do not accumulate multiple actions for the same event,
+        // which can cause the action to be called multiple times when the button is tapped.
+        // By removing all existing actions first, we guarantee that only the new action
+        // will be associated with the .touchUpInside event.
+        removeTarget(nil, action: nil, for: .touchUpInside)
         addAction(action, for: .touchUpInside)
 
         showsLargeContentViewer = true

BrowserKit/Tests/ToolbarKitTests/BrowserAddressToolbarTests.swift

@@ -0,0 +1,86 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/
+
+import XCTest
+@testable import ToolbarKit
+
+final class BrowserAddressToolbarTests: XCTestCase {
+    private var sut: BrowserAddressToolbar?
+    private var toolbarElement: ToolbarElement?
+    private var toolbarElement2: ToolbarElement?
+
+    override func setUp() {
+        super.setUp()
+        sut = BrowserAddressToolbar()
+
+        toolbarElement = ToolbarElement(
+            iconName: "icon",
+            isEnabled: true,
+            isSelected: false,
+            a11yLabel: "Test Button",
+            a11yHint: nil,
+            a11yId: "a11yID-1",
+            a11yCustomActionName: nil,
+            a11yCustomAction: nil,
+            hasLongPressAction: false,
+            onSelected: nil,
+            onLongPress: nil
+        )
+
+        toolbarElement2 = ToolbarElement(
+            iconName: "icon2",
+            isEnabled: true,
+            isSelected: false,
+            a11yLabel: "Test Button2",
+            a11yHint: nil,
+            a11yId: "a11yID-2",
+            a11yCustomActionName: nil,
+            a11yCustomAction: nil,
+            hasLongPressAction: false,
+            onSelected: nil,
+            onLongPress: nil
+        )
+    }
+
+    override func tearDown() {
+        sut = nil
+        toolbarElement = nil
+        toolbarElement2 = nil
+        super.tearDown()
+    }
+
+    func testGetToolbarButton_CreatesAndReturnsTheCachedButton() {
+        // First call to getToolbarButton should create a new button
+        guard let toolbarElement else { return }
+        let button1 = sut?.getToolbarButton(for: toolbarElement)
+        XCTAssertNotNil(button1, "Button should not be nil.")
+
+        // Second call to getToolbarButton should return the cached button
+        let button2 = sut?.getToolbarButton(for: toolbarElement)
+        XCTAssertNotNil(button2, "Button should not be nil.")
+
+        // Verify that the same button instance is returned
+        XCTAssertEqual(button1, button2, "The button should be cached and reused.")
+
+        // Verify the cache count
+        XCTAssertEqual(sut?.cachedButtonReferences.count, 1, "Cache should contain one button.")
+    }
+
+    func testGetToolbarButton_CreatesNewButtonForDifferentElements() {
+        guard let toolbarElement, let toolbarElement2 else { return }
+        // First call to getToolbarButton should create a new button for the first element
+        let button1 = sut?.getToolbarButton(for: toolbarElement)
+        XCTAssertNotNil(button1, "Button should not be nil.")
+
+        // First call to getToolbarButton should create a new button for the second element
+        let button2 = sut?.getToolbarButton(for: toolbarElement2)
+        XCTAssertNotNil(button2, "Button should not be nil.")
+
+        // Verify that different button instances are returned for different elements
+        XCTAssertNotEqual(button1, button2, "Different button instances should be created for different elements.")
+
+        // Verify the cache count
+        XCTAssertEqual(sut?.cachedButtonReferences.count, 2, "Cache should contain two buttons.")
+    }
+}