diff --git a/BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift b/BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift index 255bf7ed48dd..1d9fef4fd6ad 100644 --- a/BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift +++ b/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) diff --git a/BrowserKit/Sources/ToolbarKit/ToolbarButton.swift b/BrowserKit/Sources/ToolbarKit/ToolbarButton.swift index 545745200433..9e0e24a46b37 100644 --- a/BrowserKit/Sources/ToolbarKit/ToolbarButton.swift +++ b/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 diff --git a/BrowserKit/Tests/ToolbarKitTests/BrowserAddressToolbarTests.swift b/BrowserKit/Tests/ToolbarKitTests/BrowserAddressToolbarTests.swift new file mode 100644 index 000000000000..704551cb3dfc --- /dev/null +++ b/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.") + } +}