Skip to content

fix: correct motion bug - #112

Closed
jrpool wants to merge 8 commits into
mainfrom
motionbug
Closed

fix: correct motion bug#112
jrpool wants to merge 8 commits into
mainfrom
motionbug

Conversation

@jrpool

@jrpool jrpool commented Aug 23, 2026

Copy link
Copy Markdown
Member

The test of the motion rule of the testaro rule engine delivers false positive results if the tested page contains details elements that are closed, because the test compares an image of the page as it exists with the catalog image, which was made only after all details elements were forced open.

This change corrects that error by opening all closed details elements before the motion image is made.

The same test also reports the fraction of all pixels that changed, but the denominator is the pixel count of the entire page. A typical page has a background of uniform color that is covered only fractionally by foreground content. If all of that content disappears or is replaced with other content in situ, the perceived change is probably about 100%, but the reported change may be about 2%. Pending a more sophisticated approximation of perceived change, the revised test reports the count of changed pixels instead of the fraction, and the ordinalSeverity computation is now based on that count.

Since the motion test now opens closed details elements, it is reclassified as contaminating in tests/testaro.js and is moved to be adjacent to other contaminating tests.

Some .ts files are observed to have appeared, but instructions in CONTRIBUTING.md do not yet advise on what to do about them, so, to avoid doing anything wrong, I have not touched those files.

Copilot AI and others added 2 commits August 23, 2026 05:04
Co-authored-by: jrpool <3364797+jrpool@users.noreply.github.com>
Co-authored-by: jrpool <3364797+jrpool@users.noreply.github.com>
Co-authored-by: jrpool <3364797+jrpool@users.noreply.github.com>
@wittjeff
wittjeff self-requested a review August 23, 2026 15:04
@wittjeff

Copy link
Copy Markdown
Collaborator

From Claude:

Requested changes

  1. Message unit bug: `Content changes spontaneously (${pixelChanges}% pixels changed)` interpolates a raw pixel count but keeps the percent sign — live output on the animated target reads 4708% pixels changed. It should be (${pixelChanges} pixels changed).

  2. tests/testaro.js is now generated. Since TypeScript Phase 2 registry: typed rule registry + tests/testaro conversion — Phase 2 complete (#73) #115 (merged after this branch's last merge from main), tests/testaro.js is compiled from tests/testaro.ts, and CI regenerates and diffs it, so a hand edit to the .js will conflict with main and then fail the drift gate. The allRules change (the contaminates: true flip and the move) needs to be made in tests/testaro.ts, with the .js regenerated by npm run build:ts. Your point about CONTRIBUTING.md not covering the .ts files yet is fair — that's on us, and I'll add a section. If you'd rather not deal with the rebase, say the word and I'll update the branch: rebase onto current main, move the allRules edit into the .ts, and fix the % — with no change to your motion logic.

@wittjeff

Copy link
Copy Markdown
Collaborator

I'm thinking it'd probably be best if you go ahead and submit PRs against .js files and I'll just automatically transfer them to the .ts and rebuild.

@jrpool

jrpool commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

I'm thinking it'd probably be best if you go ahead and submit PRs against .js files and I'll just automatically transfer them to the .ts and rebuild.

Could you explain why that is better? To me, it makes sense to submit PRs per the new CONTRIBUTING.md instructions. I’ll do that in this case, and, if you think that is dysfunctional, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants