Add FXIOS-16181 [WebCompat] Add the Website Issue picker#34535
Add FXIOS-16181 [WebCompat] Add the Website Issue picker#34535razvanlitianu wants to merge 6 commits into
Conversation
✍️ Strings UpdatedDetected changes in 💪 Quality guardian3 tests files modified. You're a champion of test coverage! 🚀 🏔️ Summit ClimberThis PR is a big climb with 916 lines changed! 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ✅ Code coverageAll new and modified files meet their coverage thresholds. Client.app: Coverage: 43.55
WebCompatReporterKit: Coverage: 87.91
Generated by 🚫 Danger Swift against e637bbe |
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
b68cbbe to
10bc19a
Compare
There was a problem hiding this comment.
Pull request overview
Adds a two-level “Site Issue” picker to the WebCompat “Report a Website Issue” sheet, implemented as a category pull-down (UIMenu) plus optional sub-option rows with checkmarks, and wires the selection intents back into the Client’s Redux state.
Changes:
- Extended
WebCompatReportViewModelwith section headers and row kinds (plain,categoryMenu,subOption) and updated the sheet to render them via typed cells and stable diffable IDs. - Added Client-side category/sub-option enums and mapping from
WebCompatReporterState→ view model sections, plus delegate intents to dispatchselectCategory/selectSubOption. - Added new localized strings and expanded unit tests for the new picker behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| firefox-ios/Shared/Strings.swift | Adds localized strings for the “Site Issue” section title and category placeholder. |
| firefox-ios/firefox-ios-tests/Tests/ClientTests/WebCompatReporter/WebCompatReporterStateTests.swift | Updates tests to validate sub-option IDs via the new WebCompatSubOption enum. |
| firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/WebCompatReportViewControllerTests.swift | Adds/updates tests for section construction and hosting the ReporterKit sheet controller. |
| firefox-ios/Client/Frontend/WebCompatReporter/Redux/WebCompatIssueCategory.swift | Replaces string sub-option IDs with typed WebCompatSubOption and introduces the enum. |
| firefox-ios/Client/Frontend/Browser/WebCompat/WebCompatReportViewController.swift | Builds picker sections from Redux state and forwards selection intents to Redux actions. |
| BrowserKit/Tests/WebCompatReporterKitTests/WebCompatReportSheetViewControllerTests.swift | Adds tests for sub-option selection delegate callbacks and typed cell dequeuing. |
| BrowserKit/Sources/WebCompatReporterKit/WebCompatReportViewModel.swift | Extends the view model with section titles, row kinds, and menu options for rendering. |
| BrowserKit/Sources/WebCompatReporterKit/WebCompatReportSheetViewController.swift | Implements header rendering, stable-id diffable data source, selection handling, and theme-driven reconfiguration. |
| BrowserKit/Sources/WebCompatReporterKit/WebCompatReporterUX.swift | Adds a chevron sizing constant used by the category menu cell. |
| BrowserKit/Sources/WebCompatReporterKit/Preview/WebCompatReportSheetPreview.swift | Updates preview to an interactive host that simulates store updates for the picker. |
| BrowserKit/Sources/WebCompatReporterKit/Cells/WebCompatSubOptionCell.swift | New cell for sub-option rows with checkmark + accessibility traits. |
| BrowserKit/Sources/WebCompatReporterKit/Cells/WebCompatCategoryMenuCell.swift | New cell implementing the full-width category pull-down button + dynamic type chevron sizing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.theme = theme | ||
| view.backgroundColor = theme.colors.layer1 | ||
| collectionView.backgroundColor = theme.colors.layer1 | ||
| collectionView.setCollectionViewLayout(makeLayout(backgroundColor: theme.colors.layer1), animated: false) | ||
| navigationController?.navigationBar.tintColor = theme.colors.actionPrimary |
There was a problem hiding this comment.
Fixed. Moved self.theme = theme above the guard isViewLoaded so it short-circuits before any view/collectionView access. viewDidLoad calls applyTheme itself, so an early call just stores the theme and the colors apply once the view loads.
| for section in viewModel.sections { | ||
| snapshot.appendSections([section]) | ||
| snapshot.appendItems(section.rows, toSection: section) | ||
| sectionsByID[section.id] = section | ||
| snapshot.appendSections([section.id]) | ||
| for row in section.rows { rowsByID[row.id] = row } | ||
| snapshot.appendItems(section.rows.map { $0.id }, toSection: section.id) |
There was a problem hiding this comment.
Leaving this one out. The ids are set entirely by the Client mapping (fixed section ids plus unique category and sub-option row ids), so duplicates can't arise here. And NSDiffableDataSourceSnapshot already traps on duplicate item identifiers, so a dup would fail loudly rather than mis-render silently. Happy to add asserts if you'd rather have the contract spelled out.
| /// Issue categories for the "Report a Website Issue" form. Raw values are Glean | ||
| /// `broken-site-report` reason keys, not display copy. |
There was a problem hiding this comment.
Good catch, the comment was wrong. The category cases have no explicit raw values, so they're camelCase identifiers, not Glean reason keys. The reason keys are the snake_case WebCompatSubOption.rawValues. Reworded the doc to call the category a grouping and moved the Glean-key note onto WebCompatSubOption.
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
10bc19a to
acfa203
Compare
|
It looks like some of the added test files are missing some test coverage. Can we add some? |
|
Thanks for the review @Cramsden. The coverage is there; the gate just wasn't seeing it. The issue is that the coverage gate builds I confirmed this by temporarily running the package tests in the Danger step and merging their coverage. With that change, the gate passed and |
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
2f2ba94 to
acfa203
Compare
|
Flagging this for infra before I take you off the reviewers. Nothing to review in this PR, I just wanted the context on record. This PR tripped the code-coverage gate with the I validated a potential fix by temporarily running the package tests in the Danger step and merging their coverage into |
razvanlitianu
added
the
ignore-code-coverage
|
This pull request has conflicts when rebasing. Could you fix it @razvanlitianu? 🙏 |
Add the two-level Site Issue picker to the report sheet: a native UIMenu category pull-down and sub-option rows with a checkmark on the selected one. - Extend WebCompatReportViewModel.Row with a Kind (plain / categoryMenu / subOption) and Section headers; render via dedicated WebCompatCategoryMenuCell and WebCompatSubOptionCell keyed by stable ids in the diffable data source. - Add didSelectCategory/didSelectSubOption sheet delegate intents; the Client maps WebCompatReporterState to the view model and dispatches selectCategory/ selectSubOption. - Introduce WebCompatSubOption enum (raw values = Glean reason keys); category/ sub-option display titles map from the staged Strings in the Client. - Accessibility: selected/button/header traits, decorative glyphs hidden from VoiceOver, chevron scales with Dynamic Type.
Experiment for FXIOS-16181 to confirm the coverage gate can see the WebCompatReporterKit test bundle. Runs WebCompatReporterKitTests in the Danger step and merges its coverage into coverage.json. To be reverted.
Reverts 2f2ba94. Kept the experiment commit above as a reference for infra: running WebCompatReporterKitTests in the Danger step and merging their coverage into coverage.json flipped the code-coverage gate green (WebCompatReporterKit 87.65%). The real fix belongs at the CI level, not in this feature PR, so it's reverted here.
…, large sheet title
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
d92e972 to
d60f399
Compare
|
|
||
| nonisolated func handleNotifications(_ notification: Notification) { | ||
| guard notification.name == UIContentSizeCategory.didChangeNotification else { return } | ||
| DispatchQueue.main.async { [self] in |
There was a problem hiding this comment.
Can we use ensureMainThread here?
There was a problem hiding this comment.
Done, switched to ensureMainThread.
| /// Keeps the chevron and the button's reserved trailing space in step with | ||
| /// the current Dynamic Type size. | ||
| private func applyScaledMetrics() { | ||
| let size = scaledChevronSize |
There was a problem hiding this comment.
nit: do we need a new variable here?
There was a problem hiding this comment.
Done, inlined scaledChevronSize so there's no local.
|
|
||
| /// Preview host that fakes the store loop: holds the selection and reconfigures | ||
| /// the sheet on each intent, so the canvas behaves like the connected screen. | ||
| private final class WebCompatPickerPreviewController: UINavigationController, WebCompatReportSheetDelegate { |
There was a problem hiding this comment.
Can you elaborate on why we need all of this complicated code to just create the view for preview?
There was a problem hiding this comment.
Fair, that was left over from making the picker tappable in the canvas. Simplified it to two plain #Previews (placeholder and a selected category with sub-options) using small section builders, no state host.
- Use ensureMainThread instead of DispatchQueue.main.async in the cell's content-size handler. - Mutate menuButton.configuration in place instead of a temp copy. - Replace the interactive preview host with two static #Preview states.
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
424929f to
f1108a7
Compare
|
|
These dark mode colors feel a little off.. Is there a figma for these changes?
|
razvanlitianu
removed
the
ignore-code-coverage
Good catch. The cells were falling back to the system grouped background instead of the theme's card color, which made them look out of place in dark mode. I've switched them to |
…version - Give the picker cells .listGroupedCell() + theme.colors.layer5 so cards use the theme card color in dark mode (matches the Translations cells). - Set up WebCompatReporterKit for code coverage per the BrowserKit package guide: add CodeCoverageTargets/Testables to its scheme and wire it into Fennec.xcscheme (build-for-testing, code coverage, test plan). - Bump the staged IssueSection strings to .v154 for the current train.
razvanlitianu
force-pushed
the
rlitianu/fxios-16181-webcompat-issue-picker
branch
from
439f99f to
e637bbe
Compare
|
I'll let Carson do the review of this PR, I only commented for the small Strings comment and the unit tests code coverage stuff. Those are fixed so all good on my end 👍 |
📜 Tickets
Jira ticket
Github issue
💡 Description
Add the two-level Site Issue picker to the report sheet: a native UIMenu category pull-down and sub-option rows with a checkmark on the selected one.
🧪 Testing
To review in isolation: select the WebCompatReporterKit scheme on an iOS 26.5 simulator and open the #Preview ("Site Issue picker") in WebCompatReportSheetPreview.swift — it's interactive (open the pull-down, switch category, tap sub-options). Note the chevron/checkmark render blank in the package preview canvas (app asset catalog only) but appear in Fennec.
🎥 Demos
Demo
📝 Checklist