Skip to content

Remove FXIOS-14618 [Performance] Remove layout calls and a keyboard overlay view #31735

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

Jump to bottom
Open
Cramsden wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove FXIOS-14618 [Performance] Remove layout calls and a keyboard overlay view #31735

Cramsden wants to merge 2 commits into from
+0 −21

Conversation

@Cramsden
Copy link
Contributor

@Cramsden Cramsden commented Jan 20, 2026

📜 Tickets

Jira ticket

💡 Description

Remove layoutIfNeeded calls that do not seem to be needed as well as removing a keyboard backdrop view that does not seem to impact the animation.

🎥 Demos

Before After
Demo

📝 Checklist

  • I filled in the ticket numbers and a description of my work
  • I updated the PR name to follow our PR naming guidelines
  • I ensured unit tests pass and wrote tests for new code
  • If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
  • If adding telemetry, I read the data stewardship requirements and will request a data review
  • If adding or modifying strings, I read the guidelines and will request a string review from l10n
  • If needed, I updated documentation and added comments to complex code

@Cramsden Cramsden requested a review from a team as a code owner January 20, 2026 22:38
@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented Jan 20, 2026 •
edited
Loading

Messages
📖 Project coverage: 38.98%

🧹 Tidy commit

Just 3 file(s) touched. Thanks for keeping it clean and review-friendly!

🎉 BrowserViewController got smaller

Nice! BrowserViewController.swift got smaller by 17 lines.

✅ Per-file coverage

All changed files meet the threshold of 35.0%.

Client.app: Coverage: 37.42

File Coverage
BrowserViewController.swift 29.84% ⚠️
BaseAlphaStackView.swift 29.9% ⚠️

Generated by 🚫 Danger Swift against f241117

FilippoZazzeroni
FilippoZazzeroni reviewed Jan 21, 2026
View reviewed changes
Copy link
Collaborator

@FilippoZazzeroni FilippoZazzeroni left a comment

Choose a reason for hiding this comment

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

LGTM @Cramsden just a comment around updateToolbarLayout

BrowserKit/Sources/ToolbarKit/AddressToolbar/BrowserAddressToolbar.swift
guard !areAllStacksAlreadyVisible else { return }

let isAnimationEnabled = !UIAccessibility.isReduceMotionEnabled && animated

Copy link
Collaborator

@FilippoZazzeroni FilippoZazzeroni Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

self.layoutIfNeed is needed during the animation to animate the constraints changes on the LocationView otherwise it will result in junky animation. i've updated this code in this PR to make the call to this method less frequent only when there are some stacks that aren't visible.

Copy link
Contributor Author

@Cramsden Cramsden Jan 21, 2026

Choose a reason for hiding this comment

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

ah ok! I didn't notice the animation difference.

@thatswinnie
Copy link
Collaborator

thatswinnie commented Jan 21, 2026

@Cramsden should these changes be behind a feature flag maybe?

lmarceau
lmarceau reviewed Jan 21, 2026
View reviewed changes
firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift

guard let searchController = self.searchController else { return }

// This needs to be added to ensure during animation of the keyboard,
Copy link
Contributor

@lmarceau lmarceau Jan 21, 2026

Choose a reason for hiding this comment

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

Glad we can remove this! This was a hacky thing at the time for the legacy toolbar in 2022 😆. I pulled the branch to test that particular use case just in case and all LGTM!

@mergify
Copy link
Contributor

mergify bot commented Jan 21, 2026

This pull request has conflicts when rebasing. Could you fix it @Cramsden? 🙏

@Cramsden
Copy link
Contributor Author

Cramsden commented Jan 21, 2026

@thatswinnie it is a question I was also asking myself! I was able to test these areas of code pretty easily so I am hopeful that they are not as brittle as the bvc changes.... but I could!

@Cramsden Cramsden force-pushed the cr/FXIOS-14618_remove-unneeded-layout-calls branch from 24c2885 to 79ff418 Compare January 21, 2026 19:27
@Cramsden Cramsden force-pushed the cr/FXIOS-14618_remove-unneeded-layout-calls branch from 79ff418 to 1c91c2f Compare January 21, 2026 19:28
@thatswinnie
Copy link
Collaborator

thatswinnie commented Jan 23, 2026

@Cramsden maybe to be sure add the feature flag? Better save than sorry I guess. We could remove it after a month in the wild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmarceau lmarceau lmarceau left review comments

@FilippoZazzeroni FilippoZazzeroni FilippoZazzeroni left review comments

@thatswinnie thatswinnie Awaiting requested review from thatswinnie

@PARAIPAN9 PARAIPAN9 Awaiting requested review from PARAIPAN9

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Cramsden @mobiletest-ci-bot @thatswinnie @lmarceau @FilippoZazzeroni