Skip to content

Refactor NO_TICKET Various improvements #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 2 commits into
base: coderabbit_full_base_refactor_no_ticket_various_improvements_pr11
Choose a base branch
from
Open

Refactor NO_TICKET Various improvements #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b45c054
Apply PR changes (squashed due to cherry-pick conflicts)
tomerqodo Jan 25, 2026
89f8cbc
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import Common
/// Simple address toolbar implementation.
/// +-------------+--------------------------------------------------------+----------+
/// | navigation | [ leading ] indicators url [ trailing ] | browser |
/// | actions | [ page ] [ page ] | browser |
/// | | [ actions ] [ actions ] | actions |
/// | actions | [ page ] [ page ] | actions |
/// | | [ actions ] [ actions ] | |
/// +-------------+--------------------------------------------------------+----------+
public class BrowserAddressToolbar: UIView,
Notifiable,
Expand Down Expand Up @@ -48,7 +48,7 @@ public class BrowserAddressToolbar: UIView,

private lazy var leadingPageActionStack: UIStackView = .build()

private lazy var pageActionStack: UIStackView = .build { view in
private lazy var trailingPageActionStack: UIStackView = .build { view in
view.spacing = UX.actionSpacing
}

Expand Down Expand Up @@ -123,7 +123,7 @@ public class BrowserAddressToolbar: UIView,
trailingSpace: CGFloat,
isUnifiedSearchEnabled: Bool,
animated: Bool) {
[navigationActionStack, leadingPageActionStack, pageActionStack, browserActionStack].forEach {
[navigationActionStack, leadingPageActionStack, trailingPageActionStack, browserActionStack].forEach {
$0.isHidden = config.uxConfiguration.scrollAlpha.isZero
}
if #available(iOS 26.0, *) {
Expand Down Expand Up @@ -193,7 +193,7 @@ public class BrowserAddressToolbar: UIView,
locationContainer.addSubview(leadingPageActionStack)
locationContainer.addSubview(locationView)
locationContainer.addSubview(locationDividerView)
locationContainer.addSubview(pageActionStack)
locationContainer.addSubview(trailingPageActionStack)

toolbarContainerView.addSubview(navigationActionStack)
toolbarContainerView.addSubview(locationContainer)
Expand All @@ -214,7 +214,7 @@ public class BrowserAddressToolbar: UIView,

[navigationActionStack,
leadingPageActionStack,
pageActionStack,
trailingPageActionStack,
browserActionStack].forEach(setZeroWidthConstraint)

toolbarTopBorderHeightConstraint = toolbarTopBorderView.heightAnchor.constraint(equalToConstant: 0)
Expand Down Expand Up @@ -269,12 +269,12 @@ public class BrowserAddressToolbar: UIView,
locationView.bottomAnchor.constraint(equalTo: locationContainer.bottomAnchor),

locationDividerView.topAnchor.constraint(equalTo: locationContainer.topAnchor),
locationDividerView.trailingAnchor.constraint(equalTo: pageActionStack.leadingAnchor),
locationDividerView.trailingAnchor.constraint(equalTo: trailingPageActionStack.leadingAnchor),
locationDividerView.bottomAnchor.constraint(equalTo: locationContainer.bottomAnchor),

pageActionStack.topAnchor.constraint(equalTo: locationContainer.topAnchor),
pageActionStack.trailingAnchor.constraint(equalTo: locationContainer.trailingAnchor),
pageActionStack.bottomAnchor.constraint(equalTo: locationContainer.bottomAnchor),
trailingPageActionStack.topAnchor.constraint(equalTo: locationContainer.topAnchor),
trailingPageActionStack.trailingAnchor.constraint(equalTo: locationContainer.trailingAnchor),
trailingPageActionStack.bottomAnchor.constraint(equalTo: locationContainer.bottomAnchor),

browserActionStack.topAnchor.constraint(equalTo: toolbarContainerView.topAnchor),
browserActionStack.bottomAnchor.constraint(equalTo: toolbarContainerView.bottomAnchor),
Expand Down Expand Up @@ -319,7 +319,7 @@ public class BrowserAddressToolbar: UIView,

// Page actions
updateActionStack(stackView: leadingPageActionStack, toolbarElements: config.leadingPageActions)
updateActionStack(stackView: pageActionStack, toolbarElements: config.trailingPageActions)
updateActionStack(stackView: trailingPageActionStack, toolbarElements: config.trailingPageActions)

updateActionSpacing(uxConfig: config.uxConfiguration)
updateToolbarLayout(animated: animated)
Expand All @@ -329,7 +329,7 @@ public class BrowserAddressToolbar: UIView,
let stacks = browserActionStack.arrangedSubviews +
navigationActionStack.arrangedSubviews +
leadingPageActionStack.arrangedSubviews +
pageActionStack.arrangedSubviews
browserActionStack.arrangedSubviews
let isAnimationEnabled = !UIAccessibility.isReduceMotionEnabled && animated
Comment on lines 329 to 333
Copy link

@coderabbitai coderabbitai bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Include trailing page actions in animation list.
trailingPageActionStack is omitted and browserActionStack appears twice, so trailing page action buttons can remain at alpha 0 after updates.

🛠️ Proposed fix 
         let stacks = browserActionStack.arrangedSubviews +
                      navigationActionStack.arrangedSubviews +
                      leadingPageActionStack.arrangedSubviews +
-                     browserActionStack.arrangedSubviews
+                     trailingPageActionStack.arrangedSubviews
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let stacks = browserActionStack.arrangedSubviews +
navigationActionStack.arrangedSubviews +
leadingPageActionStack.arrangedSubviews +
pageActionStack.arrangedSubviews
browserActionStack.arrangedSubviews
let isAnimationEnabled = !UIAccessibility.isReduceMotionEnabled && animated
let stacks = browserActionStack.arrangedSubviews +
navigationActionStack.arrangedSubviews +
leadingPageActionStack.arrangedSubviews +
trailingPageActionStack.arrangedSubviews
let isAnimationEnabled = !UIAccessibility.isReduceMotionEnabled && animated
🤖 Prompt for AI Agents 
In `@BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift`
around lines 329 - 333, The stacks array mistakenly includes browserActionStack
twice and omits trailingPageActionStack, so trailing page action views don't get
animated; update the assembly of stacks (the variable created where
browserActionStack.arrangedSubviews + navigationActionStack.arrangedSubviews +
leadingPageActionStack.arrangedSubviews + browserActionStack.arrangedSubviews is
built) to include trailingPageActionStack.arrangedSubviews instead of the
duplicate browserActionStack, ensuring all page action stacks are part of the
animation list used with isAnimationEnabled.


if isAnimationEnabled {
Expand Down Expand Up @@ -416,7 +416,7 @@ public class BrowserAddressToolbar: UIView,
leadingLocationContainerConstraint?.constant = hasNavigationActions && isRegular ? -UX.horizontalSpace : 0

// Page action spacing
let hasPageActions = !pageActionStack.arrangedSubviews.isEmpty
let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty
dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0
}
Comment on lines 418 to 421
Copy link

@coderabbitai coderabbitai bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Divider visibility should follow trailing page actions.
The divider sits between the location view and the trailing page action stack, so it should reflect whether trailing actions exist.

🛠️ Proposed fix 
-        let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty
+        let hasPageActions = !trailingPageActionStack.arrangedSubviews.isEmpty
         dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0
🤖 Prompt for AI Agents 
In `@BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift`
around lines 418 - 421, The divider visibility currently checks
leadingPageActionStack; update the logic to reflect trailing actions instead:
compute hasPageActions using trailingPageActionStack.arrangedSubviews.isEmpty
and set dividerWidthConstraint?.constant to
uxConfig.browserActionsAddressBarDividerWidth when trailing actions exist
(otherwise 0). Ensure you update the reference to trailingPageActionStack and
leave uxConfig.browserActionsAddressBarDividerWidth and dividerWidthConstraint
usage unchanged.


Expand Down
7 changes: 7 additions & 0 deletions firefox-ios/Client/Application/AccessibilityIdentifiers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ struct AccessibilityIdentifiers {
struct WebView {
static let documentLoadingLabel = "WebView.documentLoadingLabel"
}

static let overKeyboardContainer = "Browser.overKeyboardContainer"
static let headerContainer = "Browser.headerContainer"
static let bottomContainer = "Browser.bottomContainer"
static let bottomContentStackView = "Browser.bottomContentStackView"
static let contentContainer = "Browser.contentContainer"
static let statusBarOverlay = "Browser.statusBarOverlay"
}

struct ContextualHints {
Expand Down
21 changes: 16 additions & 5 deletions firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ class BrowserViewController: UIViewController,
// MARK: Lazy loading UI elements
private var documentLoadingView: TemporaryDocumentLoadingView?
private(set) lazy var mailtoLinkHandler = MailtoLinkHandler()
private lazy var statusBarOverlay: StatusBarOverlay = .build { _ in }
private lazy var statusBarOverlay: StatusBarOverlay = .build { view in
view.accessibilityIdentifier = AccessibilityIdentifiers.Browser.statusBarOverlay
}
private var statusBarOverlayConstraints = [NSLayoutConstraint]()
private(set) lazy var addressToolbarContainer: AddressToolbarContainer = .build(nil, {
AddressToolbarContainer(isMinimalAddressBarEnabled: self.isMinimalAddressBarEnabled,
Expand All @@ -144,11 +146,15 @@ class BrowserViewController: UIViewController,
private(set) lazy var overlayManager: OverlayModeManager = DefaultOverlayModeManager()

// Header stack view can contain the top url bar, top reader mode, top ZoomPageBar
private(set) lazy var header: BaseAlphaStackView = .build { _ in }
private(set) lazy var header: BaseAlphaStackView = .build { view in
view.accessibilityIdentifier = AccessibilityIdentifiers.Browser.headerContainer
}

// OverKeyboardContainer stack view contains
// the bottom reader mode, the bottom url bar and the ZoomPageBar
private(set) lazy var overKeyboardContainer: BaseAlphaStackView = .build { _ in }
private(set) lazy var overKeyboardContainer: BaseAlphaStackView = .build { view in
view.accessibilityIdentifier = AccessibilityIdentifiers.Browser.overKeyboardContainer
}

// Constraints used to show/hide toolbars
var headerTopConstraint: Constraint?
Expand All @@ -170,16 +176,21 @@ class BrowserViewController: UIViewController,
}

// BottomContainer stack view contains toolbar
lazy var bottomContainer: BaseAlphaStackView = .build { _ in }
lazy var bottomContainer: BaseAlphaStackView = .build { view in
view.accessibilityIdentifier = AccessibilityIdentifiers.Browser.bottomContainer
}

// Alert content that appears on top of the content
// ex: Find In Page, SnackBar from LoginsHelper
private(set) lazy var bottomContentStackView: BaseAlphaStackView = .build { stackview in
stackview.isClearBackground = true
stackview.accessibilityIdentifier = AccessibilityIdentifiers.Browser.bottomContentStackView
}

// The content container contains the homepage, error page or webview. Embedded by the coordinator.
private(set) lazy var contentContainer: ContentContainer = .build { _ in }
private(set) lazy var contentContainer: ContentContainer = .build { view in
view.accessibilityIdentifier = AccessibilityIdentifiers.Browser.contentContainer
}

// A view for displaying a preview of the web page.
private lazy var webPagePreview: TabWebViewPreview = .build {
Expand Down
35 changes: 19 additions & 16 deletions firefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ final class AddressToolbarContainer: UIView,
guard #available(iOS 26.0, *), let windowUUID else { return 0 }

let isEditingAddress = state?.addressToolbar.isEditing == true
let shoudShowKeyboard = state?.addressToolbar.shouldShowKeyboard
let shouldShowKeyboard = state?.addressToolbar.shouldShowKeyboard
let isBottomToolbar = state?.toolbarPosition == .bottom
let shouldAdjustForAccessory = hasAccessoryView &&
!isEditingAddress &&
Expand All @@ -231,7 +231,7 @@ final class AddressToolbarContainer: UIView,

/// We want to check here if the keyboard accessory view state has changed
/// To avoid spamming redux actions.
guard hasAccessoryView != shoudShowKeyboard else { return accessoryViewOffset }
guard hasAccessoryView != shouldShowKeyboard else { return accessoryViewOffset }
store.dispatch(
ToolbarAction(
shouldShowKeyboard: hasAccessoryView,
Expand Down Expand Up @@ -445,7 +445,7 @@ final class AddressToolbarContainer: UIView,
insertSubview(leftSkeletonAddressBar, aboveSubview: toolbar)
insertSubview(rightSkeletonAddressBar, aboveSubview: toolbar)

toolbar.leadingAnchor.constraint(equalTo: leftSkeletonAddressBar.trailingAnchor).isActive = true
toolbar.leadingAnchor.constraint(equalTo: rightSkeletonAddressBar.trailingAnchor).isActive = true
toolbar.trailingAnchor.constraint(equalTo: rightSkeletonAddressBar.leadingAnchor).isActive = true
} else {
toolbar.leadingAnchor.constraint(equalTo: leadingAnchor).isActive = true
Expand All @@ -459,19 +459,22 @@ final class AddressToolbarContainer: UIView,
}

private func setupSkeletonAddressBarsLayout() {
if toolbarHelper.isSwipingTabsEnabled {
NSLayoutConstraint.activate([
leftSkeletonAddressBar.topAnchor.constraint(equalTo: topAnchor),
leftSkeletonAddressBar.trailingAnchor.constraint(equalTo: leadingAnchor),
leftSkeletonAddressBar.bottomAnchor.constraint(equalTo: bottomAnchor),
leftSkeletonAddressBar.widthAnchor.constraint(equalTo: widthAnchor, constant: -UX.skeletonBarWidthOffset),

rightSkeletonAddressBar.topAnchor.constraint(equalTo: topAnchor),
rightSkeletonAddressBar.leadingAnchor.constraint(equalTo: trailingAnchor),
rightSkeletonAddressBar.bottomAnchor.constraint(equalTo: bottomAnchor),
rightSkeletonAddressBar.widthAnchor.constraint(equalTo: widthAnchor, constant: -UX.skeletonBarWidthOffset)
])
}
guard toolbarHelper.isSwipingTabsEnabled else { return }

// Check if the interface is in landscape mode for layout adjustments
let isLandscape = UIApplication.shared.statusBarOrientation.isLandscape

NSLayoutConstraint.activate([
leftSkeletonAddressBar.topAnchor.constraint(equalTo: topAnchor),
leftSkeletonAddressBar.trailingAnchor.constraint(equalTo: leadingAnchor),
leftSkeletonAddressBar.bottomAnchor.constraint(equalTo: bottomAnchor),
leftSkeletonAddressBar.widthAnchor.constraint(equalTo: widthAnchor, constant: -UX.skeletonBarWidthOffset),

rightSkeletonAddressBar.topAnchor.constraint(equalTo: topAnchor),
rightSkeletonAddressBar.leadingAnchor.constraint(equalTo: trailingAnchor),
rightSkeletonAddressBar.bottomAnchor.constraint(equalTo: bottomAnchor),
rightSkeletonAddressBar.widthAnchor.constraint(equalTo: widthAnchor, constant: -UX.skeletonBarWidthOffset)
])
}

private func updateProgressBarPosition(_ position: AddressToolbarPosition) {
Expand Down