Skip to content

Refactor NO_TICKET Various improvements #21

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

Refactor NO_TICKET Various improvements #21

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
Copy link

@cursor cursor bot Jan 21, 2026

Choose a reason for hiding this comment

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

Duplicate browserActionStack causes missing alpha animation

Medium Severity

The stacks variable includes browserActionStack.arrangedSubviews twice but omits trailingPageActionStack.arrangedSubviews. This appears to be a copy-paste error during the rename from pageActionStack to trailingPageActionStack. As a result, the trailing page action buttons won't have their alpha set to 1.0 during toolbar animations, leaving them invisible after animation completes.

Fix in Cursor Fix in Web

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
Copy link

@cursor cursor bot Jan 21, 2026

Choose a reason for hiding this comment

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

Wrong stack checked for divider width calculation

Medium Severity

The hasPageActions check was changed from the renamed trailingPageActionStack to leadingPageActionStack. The locationDividerView is positioned between locationView and trailingPageActionStack, so the divider width should be set based on whether there are trailing page actions, not leading ones. This may cause the divider to show/hide incorrectly.

Fix in Cursor Fix in Web

dividerWidthConstraint?.constant = hasPageActions ? uxConfig.browserActionsAddressBarDividerWidth : 0
}

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
Copy link

@cursor cursor bot Jan 21, 2026

Choose a reason for hiding this comment

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

Toolbar leading constraint uses wrong skeleton anchor

High Severity

The toolbar's leadingAnchor constraint was changed from leftSkeletonAddressBar.trailingAnchor to rightSkeletonAddressBar.trailingAnchor. Since the right skeleton is positioned outside the container's trailing edge, this constraint places the toolbar's leading edge beyond the right skeleton, creating an impossible or broken layout with the toolbar potentially having zero or negative width.

Fix in Cursor Fix in Web

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