Repository navigation
fix: align camera reset accessible names with visible labels - #87
Conversation
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Review done by claude-opus-5-5 (adversarial to codex)
This single commit renames the aria-label of the two 3D camera reset buttons so each starts with the visible text "Reset View". It also adds a test that the accessible name contains the visible text and updates the camera-control test to use the new name. No defects were found: the label change is markup only, it does not touch resetCamera behavior, and no file at the head SHA still uses the old names.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 0m 55s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
src/template.pytests/browser/accessibility.spec.jstests/browser/controls.spec.js
- Context read:
tests/browser/fixture.jstests/browser/layout.spec.jstests/browser/descriptions-walkthrough.spec.jstests/browser/walkthrough.spec.jstests/browser/mobile-walkthrough.spec.jstests/browser/report.spec.jstests/browser/static-report.spec.jstests/browser/fallbacks.spec.jstests/browser/integrity.spec.jstests/browser/landing.spec.jstests/browser/dune-build-editor.spec.jstests/browser/dune-build-files.spec.jstests/performance/budget.test.jspackage.json.github/workflows/validate.ymlREADME.md
- Verified:
- The compare response's
base_commit.shaequals2153f1b7acbd8db2951552c103f7c8c7cc23c332and its last commit equals5f89f2109809ffd37071478b65a33865839e9996. The compare status isahead, ahead by 1 and behind by 0, andmerge_baseequals the base. - The diff touches 3 files and only changes two
aria-labelstrings plus test locators and assertions. TheonclickhandlersresetCamera('vel')andresetCamera('cad')are unchanged. - Both new labels in
src/template.py(lines 1082 and 1100) start with the visible button textReset View. This matches the intent of axe's label-content-name-mismatch rule. src/template.pycontains exactly two buttons labeled "Reset View", which matches the new assertiontoHaveCount(2).- No test or doc file at the head SHA still references the old names "Reset weekly velocity camera" or "Reset day and hour cadence camera".
controls.spec.jsline 23 uses the new name. - The new loop in
accessibility.spec.jsiterates over the same locator whose count is asserted right after it, so if no buttons match, the count check still fails. - The new regex is built from the visible text "Reset View", which has no regex metacharacters.
- The PR's claim of "four tests" matches the files:
accessibility.spec.jshas 3 tests andcontrols.spec.jshas 1.
- The compare response's
- Not verified:
- I did not run the tests or axe, so whether they pass at runtime is unconfirmed.
- The PR says axe 4.14.0 flagged the old labels.
package.jsonpins@axe-core/playwrightto 4.13.0, so CI probably runs a different axe version than the one the PR used. The version-independent accessible-name check the PR adds covers this gap. - I did not view the screenshots in the PR body.
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 5f89f2109809ffd37071478b65a33865839e9996 • SFL v0
5f89f21 to
47d9a69
Compare
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 2
Review done by claude-opus-5-5 (adversarial to codex)
The two camera reset buttons now have accessible names that start with their visible text, "Reset View". The browser test, the benchmark selector and a new regression check in the accessibility spec were updated to match. I found no Critical, High, Medium or Low defect: every reference to the old names in the reviewed files was updated, the reset behavior is unchanged, and the new check catches a regression on either button.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 0m 54s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
src/template.pytests/browser/accessibility.spec.jstests/browser/controls.spec.jsscripts/benchmark_browser.js
- Context read:
package.jsontests/test_template.pytests/browser/walkthrough.spec.jstests/browser/mobile-walkthrough.spec.jstests/browser/descriptions-walkthrough.spec.jstests/browser/report.spec.jstests/browser/static-report.spec.jstests/browser/layout.spec.jstests/browser/fallbacks.spec.jstests/browser/fixture.jstests/browser/integrity.spec.jstests/test_static_editions.pytests/performance/budget.test.jsscripts/verify_budget_probe.jssrc/landing.pyquality/browser-performance.mdREADME.md
- Verified:
- Compare
base_commit.shais93c369d8cd708cd2c66529f70325efeab88111e5, the last commit isd6619b17eefb1180f39c3fd41bd0b2074cbde87b, the merge base equals the base,ahead_byis 2 andbehind_byis 0 - The PR's
headRefOidandbaseRefOidmatch the reserved SHAs - After the change, src/template.py has no copy of the old names
Reset weekly velocity cameraorReset day and hour cadence camera, and neither do the other downloaded test, script and documentation files - Both button labels in src/template.py (lines 1082 and 1100) start with the visible text
Reset Viewand keep the chart context. TheonclickhandlersresetCamera('vel')andresetCamera('cad')are unchanged - The selectors in controls.spec.js line 23 and benchmark_browser.js line 64 exactly match the new
aria-labelon the velocity button - No
text-transformapplies to.three-btnor its.three-controls/.three-headerancestors, soinnerTextreturnsReset Viewand the case-sensitivenew RegExp(innerText)check holds - The new
toHaveCount(2)assertion fails if either button's name stops starting withReset View, so the regression check is meaningful resetCamera(around line 1698) is unchanged and still restores the camera position that controls.spec.js expects
- Compare
- Not verified:
- Runtime results for the browser tests, axe scan and benchmark: execution was forbidden
- The PR's claim that axe-core 4.14.0 flags the old names: package.json pins
@axe-core/playwright4.13.0, and the rule's behavior in either version was not checked - The committed files under editions/ were not searched for the old names: their size and separate generation path put them outside this change
- Screenshot content linked in the PR body was not inspected
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head d6619b17eefb1180f39c3fd41bd0b2074cbde87b • SFL v0
The camera reset controls display “Reset View”, but their accessible names omit “View”. Axe-core 4.14.0 flags both controls for label-content-name-mismatch. Start each accessible name with the full visible label and retain its chart context. Add a regression check and update the camera-control locator.
Validation
npm run fixture && npx playwright test tests/browser/accessibility.spec.js tests/browser/controls.spec.js.