WT-1327: What's next (Roadmap page) QA Fixes#1503
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1503 +/- ##
=======================================
Coverage 80.90% 80.90%
=======================================
Files 147 147
Lines 10133 10133
=======================================
Hits 8198 8198
Misses 1935 1935 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ee76345 to
3bf7e83
Compare
| } else { | ||
| section.classList.remove('hidden'); | ||
| } |
There was a problem hiding this comment.
Bug: the if branch above adds .hidden to both section and itemsList, but this else only removes it from the section. Since .hidden is the global display: none utility (flare26-utilities.css:52), once an items list is hidden it stays hidden for the rest of the page session.
Repro: select a filter that empties section A → switch to a different filter that re-populates A → A's header reappears with no items below it. Clear-all reproduces the same way.
| } else { | |
| section.classList.remove('hidden'); | |
| } | |
| } else { | |
| section.classList.remove('hidden'); | |
| if (itemsList) { | |
| itemsList.classList.remove('hidden'); | |
| } | |
| } |
| function initRoadmapAccordion() { | ||
| const mq = window.matchMedia('not (min-width: 900px)'); | ||
|
|
||
| function handleAccordion() { | ||
| if (!mq.matches) return; | ||
|
|
||
| const toggles = document.querySelectorAll('.fl-roadmap-section-toggle'); | ||
| toggles.forEach((toggle) => { | ||
| const section = toggle.closest('.fl-roadmap-list-section'); | ||
| if (!section) return; | ||
|
|
||
| toggle.addEventListener('click', () => { | ||
| const collapsed = section.classList.toggle('is-collapsed'); | ||
| toggle.setAttribute('aria-expanded', String(!collapsed)); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| handleAccordion(); | ||
| mq.addEventListener('change', handleAccordion); | ||
| } |
There was a problem hiding this comment.
Bug: each call to handleAccordion() attaches a new click listener to every toggle. The function runs on init AND on every change event where mq.matches becomes true. Listeners are never removed, so they stack on every mobile→desktop→mobile round-trip.
Repro: load page <900px → click toggle (works). Resize ≥900px → resize back <900px. Click toggle — does nothing. Two listeners each flip is-collapsed once, net zero. Each subsequent round-trip adds another listener; odd counts = one net toggle, even counts = no-op.
Suggested fix: attach listeners once at init unconditionally, and gate the behavior on mq.matches inside the handler. Drop the matchMedia change listener entirely.
| function initRoadmapAccordion() { | |
| const mq = window.matchMedia('not (min-width: 900px)'); | |
| function handleAccordion() { | |
| if (!mq.matches) return; | |
| const toggles = document.querySelectorAll('.fl-roadmap-section-toggle'); | |
| toggles.forEach((toggle) => { | |
| const section = toggle.closest('.fl-roadmap-list-section'); | |
| if (!section) return; | |
| toggle.addEventListener('click', () => { | |
| const collapsed = section.classList.toggle('is-collapsed'); | |
| toggle.setAttribute('aria-expanded', String(!collapsed)); | |
| }); | |
| }); | |
| } | |
| handleAccordion(); | |
| mq.addEventListener('change', handleAccordion); | |
| } | |
| function initRoadmapAccordion() { | |
| const mq = window.matchMedia('not (min-width: 900px)'); | |
| const toggles = document.querySelectorAll('.fl-roadmap-section-toggle'); | |
| toggles.forEach((toggle) => { | |
| const section = toggle.closest('.fl-roadmap-list-section'); | |
| if (!section) return; | |
| toggle.addEventListener('click', () => { | |
| if (!mq.matches) return; | |
| const collapsed = section.classList.toggle('is-collapsed'); | |
| toggle.setAttribute('aria-expanded', String(!collapsed)); | |
| }); | |
| }); | |
| } |
| if (clearAllButton && activeFilters.size) { | ||
| clearAllButton.disabled = false; | ||
| clearAllButton.querySelector('.fl-tag').classList.add('is-selected'); | ||
| } else { | ||
| clearAllButton.disabled = true; | ||
| clearAllButton.querySelector('.fl-tag').classList.remove('is-selected'); | ||
| } |
There was a problem hiding this comment.
The if guards clearAllButton &&, but the else immediately dereferences clearAllButton.disabled and clearAllButton.querySelector(...). Today the template always renders clear-all alongside filter buttons so this won't crash in production — but it's a latent crash if anything (pattern library, A/B variant, future template change) ever renders filter buttons without clear-all.
Tightening this up also eliminates the duplicated .fl-tag lookup:
| if (clearAllButton && activeFilters.size) { | |
| clearAllButton.disabled = false; | |
| clearAllButton.querySelector('.fl-tag').classList.add('is-selected'); | |
| } else { | |
| clearAllButton.disabled = true; | |
| clearAllButton.querySelector('.fl-tag').classList.remove('is-selected'); | |
| } | |
| if (clearAllButton) { | |
| clearAllButton.disabled = !activeFilters.size; | |
| const tag = clearAllButton.querySelector('.fl-tag'); | |
| if (tag) { | |
| tag.classList.toggle('is-selected', Boolean(activeFilters.size)); | |
| } | |
| } |
| <button | ||
| type="button" | ||
| class="fl-roadmap-section-toggle" | ||
| aria-expanded="true" |
There was a problem hiding this comment.
aria-expanded is hardcoded "true" and JS only updates it on click. Combined with the desktop CSS override at flare26-roadmap-list-section.css:307-310 that forces .fl-roadmap-list visible on desktop regardless of .is-collapsed, this leads to a screen-reader / visual mismatch:
Repro: on mobile, collapse a section (aria-expanded flips to "false", .is-collapsed added). Resize to desktop — list visually reappears via the CSS override, but aria-expanded stays "false". VoiceOver / NVDA announce the section as collapsed even though it isn't.
Cleanest fix is to reset both aria-expanded and .is-collapsed when the viewport leaves mobile. In the accordion handler (after fixing #2), add a mq.addEventListener('change', ...) that wipes the collapsed state on desktop, e.g.:
mq.addEventListener('change', (e) => {
if (!e.matches) {
toggles.forEach((toggle) => {
toggle.setAttribute('aria-expanded', 'true');
toggle.closest('.fl-roadmap-list-section')?.classList.remove('is-collapsed');
});
}
});(No suggestion block here since the fix lives in the JS, not this template line.)
| if (tag) { | ||
| tag.classList.toggle( | ||
| 'is-selected', | ||
| visible && activeFilters.has(tagEl.dataset.tag) |
There was a problem hiding this comment.
Minor: is-selected is stripped whenever the item is hidden, but the user's filter selection hasn't changed — the tag styling is then stale if anything ever re-shows the item without re-running applyFilter. Today there's no such path so it's latent, but cleaner to compute it independently:
| visible && activeFilters.has(tagEl.dataset.tag) | |
| activeFilters.has(tagEl.dataset.tag) |
|
Pulled this down and visually confirmed every change in the PR description: accordion behavior on mobile, clear-all button, primary button reorder, icon swaps, spacing tweaks, dark mode. Looks great. Left a few inline comments on the JS, couple of bugs around filter-state and accordion listeners, plus an a11y note on aria-expanded. Nothing blocking the visuals, but worth a pass before merge. |
One-line summary
This PR implements various fixes and changes to the Roadmap page. It previously had a refactor to the roadmap item buttons, but this change will be implemented in a separate PR.
Issue / Bugzilla link
https://mozilla-hub.atlassian.net/browse/WT-1327
Testing
Load the page fixtures and validate that every sub-item on the Jira ticket is implemented.