-
Notifications
You must be signed in to change notification settings - Fork 847
fix(performanceTimer): work in frames #4834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where the performanceTimer fails to work correctly in iframe environments and improves logging detail for rule gathering performance. The changes rename performance markers from "axe" to "audit" and enhance the gather performance log to include rule ID and node count information.
- Renamed performance markers from
mark_axe_starttomark_audit_startto fix iframe compatibility - Enhanced gather performance logging to include rule ID and node count for better debugging visibility
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/core/utils/performance-timer.js | Updates performance marker name from 'mark_axe_start' to 'mark_audit_start' to resolve iframe issues |
| lib/core/base/rule.js | Improves gather performance logging to include rule ID and node count with template literal formatting |
lib/core/utils/performance-timer.js
Outdated
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || | ||
| window.performance.getEntriesByName('mark_audit_start')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is kinda making me wonder if we should rethink not having tests for this. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; I think tests would be a good idea (especially since performanceTimer is a documented part of our API).
lib/core/utils/performance-timer.js
Outdated
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || | ||
| window.performance.getEntriesByName('mark_audit_start')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; I think tests would be a good idea (especially since performanceTimer is a documented part of our API).
lib/core/utils/performance-timer.js
Outdated
| // we get measures made by the page before axe ran (which is confusing) | ||
| const axeStart = | ||
| window.performance.getEntriesByName('mark_axe_start')[0]; | ||
| const { startTime } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: A few lines up, this call is defensive about the possibility of window.performance not existing at all; should it also be a bit more defensive about the possibility of the mark_*_start events not being available (eg, assert with a meaningful message that start or auditStart must be called first)?
lib/core/utils/performance-timer.js
Outdated
| const axeStart = | ||
| window.performance.getEntriesByName('mark_axe_start')[0]; | ||
| const { startTime } = | ||
| window.performance.getEntriesByName('mark_axe_start')[0] || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this actually does the right thing in the event that you're in an environment with more than one mark, since we never seem to clear marks/measures out from previous runs. I think what you'd actually want here is "grab the most recent (not 0th) of each type of mark, and if both exist, use the one with later start_time)".
dbjorge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo one comment
| it('returns the time elapsed since axe started', async () => { | ||
| performanceTimer.start(); | ||
| await sleep(100); | ||
| assert.closeTo(performanceTimer.timeElapsed(), 100, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit worried when first reading this about how reliable a 10ms epsilon would be, but I verified in a few circleci runs + 20 trials locally that it does seem like it passes reliably. 👍
| */ | ||
| clearMark(...markNames) { | ||
| for (const markName of markNames) { | ||
| window.performance.clearMarks(markName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be equally as defensive about window.performance.clearMarks not existing as mark/measure/logMeasures are (who knows what existing jsdom environment breaks if we assume it's available...)
## [4.11.0](v4.10.3...v4.11.0) (2025-10-07) ### Features - add RGAA tags to rules ([#4862](#4862)) ([53a925a](53a925a)) - **aria-prohibited-attr:** add support for fallback roles ([#4325](#4325)) ([62a19a9](62a19a9)) - **axe.d.ts:** add nodeSerializer typings ([#4551](#4551)) ([a2f3a48](a2f3a48)), closes [#4093](#4093) - **DqElement:** deprecate fromFrame function ([#4881](#4881)) ([374c376](374c376)), closes [#4093](#4093) - **DqElement:** Truncate large `html` strings when the element has a large outerHTML string ([#4796](#4796)) ([404a4fb](404a4fb)), closes [#4544](#4544) - **get-xpath:** return proper relative selector for id ([#4846](#4846)) ([1035f9e](1035f9e)), closes [#4845](#4845) - **i18n:** Add Portugal Portuguese translation ([#4725](#4725)) ([5b6a65a](5b6a65a)) - incomplete with node on which an error occurred ([#4863](#4863)) ([32ed8da](32ed8da)) - **locale:** Added ru locale ([#4565](#4565)) ([067b01d](067b01d)) - **tap:** some best practice rules map to RGAA ([#4895](#4895)) ([bc33f4c](bc33f4c)) - **td-headers-attr:** report headers attribute referencing other <td> elements as unsupported ([#4589](#4589)) ([ec7c6c8](ec7c6c8)), closes [#3987](#3987) ### Bug Fixes - **aria-allowed-role:** add form to allowed roles of form element ([#4588](#4588)) ([8aa47ac](8aa47ac)), closes [/github.com/dequelabs/axe-core/blob/develop/lib/standards/html-elms.js#L264](https://github.com/dequelabs//github.com/dequelabs/axe-core/blob/develop/lib/standards/html-elms.js/issues/L264) - **aria-allowed-role:** Add math to allowed roles for img element ([#4658](#4658)) ([95b6c18](95b6c18)), closes [#4657](#4657) - **autocomplete-valid :** Ignore readonly autocomplete field ([#4721](#4721)) ([491f4ec](491f4ec)), closes [#4708](#4708) - **autocomplete-valid:** treat values "xon" and "xoff" as non-WCAG-violations ([#4878](#4878)) ([52bc611](52bc611)), closes [#4877](#4877) - **axe.d.ts:** add typings for preload options object ([#4543](#4543)) ([cfd2974](cfd2974)) - **button-name,input-button-name,input-img-alt:** allow label to give accessible name ([#4607](#4607)) ([a9710d7](a9710d7)), closes [#4472](#4472) [#3696](#3696) [#3696](#3696) - **captions:** fix grammar in captions check incomplete message ([#4661](#4661)) ([11de515](11de515)) - **color-contrast:** do not run on elements with font-size: 0 ([#4822](#4822)) ([d77c885](d77c885)), closes [#4820](#4820) - consistently parse tabindex, following HTML 5 spec ([#4637](#4637)) ([645a850](645a850)), closes [#4632](#4632) - **core:** measure perf for async checks ([#4609](#4609)) ([7e9bacf](7e9bacf)) - fix grammar when using "alternative text" in a sentence ([#4811](#4811)) ([237a586](237a586)), closes [#4394](#4394) - **get-ancestry:** add nth-child selector for multiple siblings of shadow root ([#4606](#4606)) ([1cdd6c3](1cdd6c3)), closes [#4563](#4563) - **get-ancestry:** don't error when there is no parent ([#4617](#4617)) ([a005703](a005703)) - **locale:** fix typos in japanese (ja) locale ([#4856](#4856)) ([3462cc5](3462cc5)) - **locale:** fixed typos in german (DE) locale ([#4631](#4631)) ([b7736de](b7736de)) - **locale:** proofread and updated de.json ([#4643](#4643)) ([8060ada](8060ada)) - **meta-viewport:** lower impact to moderate ([#4887](#4887)) ([2f32aa5](2f32aa5)), closes [#4714](#4714) - **no-autoplay-audio:** don't timeout for preload=none media elements ([#4684](#4684)) ([cdc871e](cdc871e)) - **performanceTimer:** throwing in axe catch clause ([#4852](#4852)) ([a4ade04](a4ade04)), closes [/github.com/dequelabs/axe-core/blob/e7dae4ec48cbfef74de9f833fdcfb178c1002985/lib/core/base/rule.js#L297-L300](https://github.com/dequelabs//github.com/dequelabs/axe-core/blob/e7dae4ec48cbfef74de9f833fdcfb178c1002985/lib/core/base/rule.js/issues/L297-L300) - **performanceTimer:** work in frames ([#4834](#4834)) ([d7dfebc](d7dfebc)) - **rules:** Change "alternate text" to "alternative text" ([#4582](#4582)) ([b03ada3](b03ada3)) - **target-size:** do not treat focusable tabpanels as targets ([#4702](#4702)) ([60d11f2](60d11f2)), closes [#4421](#4421) [#4701](#4701) - **type:** correct RuleError type ([#4893](#4893)) ([d1aa8e2](d1aa8e2)) - **types:** correct raw types ([#4903](#4903)) ([3eade11](3eade11)) This PR was opened by a robot 🤖 🎉
Turns out our performanceTimer errors when running in iframes. I also added some detail to gather so we could see what rule is being gathered.