feat(theory): touch container vs visible glyph for tap-target check#3
Closed
viktor-savchik-idf wants to merge 2 commits into
Closed
feat(theory): touch container vs visible glyph for tap-target check#3viktor-savchik-idf wants to merge 2 commits into
viktor-savchik-idf wants to merge 2 commits into
Conversation
viktor-savchik-idf
force-pushed
the
feat/hit-area-and-tautology
branch
from
bf9fe6c to
67ed397
Compare
fitts_undersized_target now distinguishes the touch CONTAINER from the visible glyph. Element schema gains optional hit_w / hit_h fields: when declared and below the platform minimum the finding fires at HIGH (real defect); when only the visible bbox is known the finding still fires but at MEDIUM worded as "verify the touch container in code" — Compose IconButton and SwiftUI Button already wrap their content in a compliant hit area by default, so a small visible glyph is not automatically a defect. Eliminates a class of false positives that triggered on every Material IconButton with a 24dp glyph. Refactored: _check_tap_target is split out of _check_fitts so each function does one thing. Side benefit — the tap-target check now fires for a single interactive element (Fitts's "need ≥2 targets" early- return was silently skipping it). Public check names are unchanged. SKILL.md gains an Optical Alignment section documenting why asymmetric icons (pencil, arrow, play, magnifier) have visual centres offset from geometric centres, with references to Apple HIG SF Symbols, Material Iconography, and Müller-Brockmann — and why automated optical-centre checking is intentionally out of scope (high false-positive rate without per-icon optical-grid metadata). Also adds an "Icon + label tautology" inline rule that the model applies during reviews — a pictogram paired with the bare action verb it already encodes is a polish failure. Deliberately a model-applied rule rather than a code check; automated detection would need a per- locale vocabulary that is expensive to maintain and produces only LOW-severity findings. The Touch Targets section is clarified: the 48dp / 44pt minimum is for the touch container, not the visible glyph. 6 new unit tests (5 hit-area + 1 regression guard for single-element tap-target).
Adds tools/tests/fixtures/confirm_screen_with_iconbutton.json — a generic mobile confirmation screen exercising the three tap-target patterns the feat commit introduces: - Material IconButton (24dp glyph + 48dp hit area declared) — silent. - Bare undersized icon without hit_w/hit_h — MEDIUM "verify in code". - Full-width primary CTA at the bottom — silent. The accompanying smoke test asserts the exact (check, element, severity) tuples so anyone reviewing the PR can replay end-to-end behaviour deterministically.
viktor-savchik-idf
force-pushed
the
feat/hit-area-and-tautology
branch
from
67ed397 to
f103bbc
Compare
viktor-savchik-idf
changed the title
Collaborator
|
Closing in favor of #4, which combines and rewrites this work as one OSS-ready PR with project-specific language removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fitts_undersized_targetwas firing on every MaterialIconButtonwhose visible icon was below 48dp — but the 48dp minimum applies to the invisible touch container, not the glyph inside it. The check now uses optionalhit_w/hit_hfields when declared, and downgrades to MEDIUM "verify in code" when only the visible bbox is known.Refactoring landed in the same feat commit:
_check_tap_targetis split out of_check_fittsso each function does one thing. Side benefit — the tap-target check now fires for a single interactive element (Fitts'slen(interactive) < 2early-return was silently skipping it). Public check names are unchanged.SKILL.md gains two documentation sections:
Commits
feat(theory): feature + refactor + SKILL.md + CHANGELOG + 6 unit tests.test(theory): checked-in fixturetools/tests/fixtures/confirm_screen_with_iconbutton.json+ smoke test asserting exact(check, element, severity)tuples.Schema changes (optional, backward-compatible)
hit_w/hit_hSeverity matrix for
fitts_undersized_targethit_w/hit_hdeclared?Tests
test_mcp.pyasyncio-fixture failures onorigin/main).Smoke test (end-to-end, from the checked-in fixture)
Correctly silent on
back_arrow_material_iconbutton(24dp glyph wrapped in a 48dpIconButtonvia declaredhit_w/hit_h) — the false positive that motivated the PR.