Skip to content

Refactor NO_TICKET Various improvements #24

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_combined_20260121_augment_sentry_coderabbit_1_base_refactor_no_ticket_various_improvements_pr228
Choose a base branch
from
Open

Refactor NO_TICKET Various improvements #24

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
d8e70c3
Apply PR changes (squashed due to cherry-pick conflicts)
tomerqodo Jan 21, 2026
ce7cd22
update pr
tomerqodo Jan 21, 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

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 22, 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 ignores trailing page actions.

hasPageActions only checks the leading stack. If trailing actions exist but leading is empty, the divider stays hidden.

🐛 Proposed fix 
-        let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty
+        let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty ||
+                             !trailingPageActionStack.arrangedSubviews.isEmpty
📝 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
// Page action spacing
let hasPageActions = !pageActionStack.arrangedSubviews.isEmpty
let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty
dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0
}
// Page action spacing
let hasPageActions = !leadingPageActionStack.arrangedSubviews.isEmpty ||
!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 logic only checks
leadingPageActionStack, so trailing page actions can be ignored; update the
calculation of hasPageActions to consider both
leadingPageActionStack.arrangedSubviews and
trailingPageActionStack.arrangedSubviews (e.g., check that either is not empty)
and then set dividerWidthConstraint?.constant to
uxConfig.browserActionsAddressBarDividerWidth when either stack has actions,
otherwise 0; modify the code around hasPageActions, dividerWidthConstraint,
leadingPageActionStack, and trailingPageActionStack accordingly.


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
Comment on lines +448 to 449
Copy link

@coderabbitai coderabbitai bot Jan 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix inverted toolbar constraints (leading/trailing pinned to the same right skeleton).
toolbar.leadingAnchor is now tied to rightSkeletonAddressBar.trailingAnchor while toolbar.trailingAnchor is tied to rightSkeletonAddressBar.leadingAnchor, which creates an unsatisfiable/inverted width. The leading anchor should remain tied to the left skeleton’s trailing edge.

🐛 Proposed fix 
-            toolbar.leadingAnchor.constraint(equalTo: rightSkeletonAddressBar.trailingAnchor).isActive = true
+            toolbar.leadingAnchor.constraint(equalTo: leftSkeletonAddressBar.trailingAnchor).isActive = true
🤖 Prompt for AI Agents 
In `@firefox-ios/Client/Frontend/Browser/Toolbars/AddressToolbarContainer.swift`
around lines 448 - 449, The toolbar's horizontal constraints are inverted:
toolbar.leadingAnchor is incorrectly constrained to
rightSkeletonAddressBar.trailingAnchor, creating a negative/inconsistent width;
change the leading constraint to use leftSkeletonAddressBar.trailingAnchor so
the toolbar spans between leftSkeletonAddressBar.trailingAnchor and
rightSkeletonAddressBar.leadingAnchor (update the constraint involving
toolbar.leadingAnchor to reference leftSkeletonAddressBar.trailingAnchor while
keeping toolbar.trailingAnchor tied to rightSkeletonAddressBar.leadingAnchor).

} 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