Skip to content

Add FXIOS-15840 [Ad Blocker] Add ad blocker badge to menu#33889

Closed
issammani wants to merge 12 commits into
mainfrom
feat/add-ad-blocker-badge
Closed

Add FXIOS-15840 [Ad Blocker] Add ad blocker badge to menu#33889
issammani wants to merge 12 commits into
mainfrom
feat/add-ad-blocker-badge

Conversation

@issammani

@issammani issammani commented May 19, 2026

Copy link
Copy Markdown
Collaborator

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR:

Uploading Simulator Screen Recording - iPhone 17 Pro - 2026-05-19 at 15.48.52.mov…

🎥 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

@issammani issammani requested a review from Foxbolts May 19, 2026 14:13
@issammani issammani requested a review from a team as a code owner May 19, 2026 14:13
@issammani issammani changed the title Add FXIOS-15842 [Ad Blocker] Add ad blocker badge to menu Add FXIOS-15840 [Ad Blocker] Add ad blocker badge to menu May 19, 2026

@Foxbolts Foxbolts left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some a11y tweaks and a question about ad blocker settings presentation from the menu. Otherwise looks good 👍

label.numberOfLines = 0
label.lineBreakMode = .byWordWrapping
label.adjustsFontForContentSizeCategory = true
label.accessibilityTraits = .button

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should make the entire view the a11y element here instead of the label, that way during voiceover the whole view is focused.

Would remove label.accessibilityTraits = .button and add label.isAccessibilityElement = false

then add this to the init:

isAccessibilityElement = true
accessibilityTraits = .button

and accessibilityLabel = text in configure()

}

func presentAdBlockerSettings() {
let browsingSettings = BrowsingSettingsViewController(profile: profile, windowUUID: windowUUID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a parent coordinator is never set for BrowsingSettingsViewController, so the settings that open nested screens like Autoplay don't do anything here. Is this intended?

bottom: UX.siteProtectionsContentVerticalPadding,
right: UX.siteProtectionsContentHorizontalPadding)
stack.distribution = .fill
private lazy var badgesStack: UIStackView = .build { stack in

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems like for large dynamic type we will need to either add horizontal compression, or better yet, vertical stacking when dynamic type gets too large

Image

Base automatically changed from feat/add-ad-blocker-ui to main May 20, 2026 09:42
@mergify

mergify Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions!

@github-actions github-actions Bot added the stale Stalebot use this label to stale issues and PRs label Jun 4, 2026
@github-actions github-actions Bot closed this Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stalebot use this label to stale issues and PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants