-
Notifications
You must be signed in to change notification settings - Fork 788
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
fix(target-size): do not crash for nodes with many overlapping widgets #4373
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.
Couple editorials, and I think this needs an integration test.
@@ -10,7 +10,7 @@ import { | |||
* Determine if an element has a minimum size, taking into account | |||
* any elements that may obscure it. | |||
*/ | |||
export default function targetSize(node, options, vNode) { | |||
export default function targetSizeEvaluate(node, options, vNode) { |
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.
We talked about adding an early pass for elements that have 10x the required size. I don't mind that in a different PR, but I don't think we should close the issues until we have that in.
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 think I'd prefer to move that to it's own pr so we can get this in quicker. Which ticket do you think it shouldn't close?
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
## [4.9.0](v4.8.4...v4.9.0) (2024-03-25) ### Features - adding the wcag131 tag to the aria-hidden-body rule ([#4349](#4349)) ([dd4c3c3](dd4c3c3)), closes [#4315](#4315) - **checks:** deprecate aria-busy check ([#4356](#4356)) ([be0b555](be0b555)), closes [#4347](#4347) [#4340](#4340) - **color:** add color channel values and luminosity, saturation, clip functions ([#4366](#4366)) ([9e70199](9e70199)), closes [/github.com//pull/4365/files#r1517706612](https://github.com/dequelabs//github.com/dequelabs/axe-core/pull/4365/files/issues/r1517706612) - **i18n:** add Greek Translations ([#3836](#3836)) ([3ea9a48](3ea9a48)) - **i18n:** Add Italian translation ([#4344](#4344)) ([de1baa9](de1baa9)) - **i18n:** Add Simplified Chinese translation ([#4379](#4379)) ([bda7c8d](bda7c8d)) - **i18n:** Add Taiwanese Mandarin translation ([#4299](#4299)) ([c5e11de](c5e11de)) ### Bug Fixes - Add LICENSE-3RD-PARTY.txt file ([#4304](#4304)) ([daa0fe6](daa0fe6)) - add Object.values polyfill for node <=6 ([#4274](#4274)) ([5eb867b](5eb867b)) - **aria-required-children:** avoid confusing aria-busy message in failures ([#4347](#4347)) ([591607d](591607d)), closes [#fail13](https://github.com/dequelabs/axe-core/issues/fail13) [#4340](#4340) - avoid reading element-specific node properties of non-element node types ([#4317](#4317)) ([b853b18](b853b18)), closes [#4316](#4316) [#4316](#4316) - **color-contrast:** handle text that is outside `overflow: hidden` ancestor ([#4357](#4357)) ([bdb7300](bdb7300)), closes [#4253](#4253) - **color-contrast:** support color blend modes hue, saturation, color, luminosity ([#4365](#4365)) ([7ae4761](7ae4761)) - **d.ts:** RawNodesResult issues ([#4229](#4229)) ([d660518](d660518)) - **d.ts:** RunOptions.reporter can be any string ([#4218](#4218)) ([e53f5c5](e53f5c5)) - **i18n:** update Italian translations ([#4377](#4377)) ([4d65d4b](4d65d4b)) - **listitem:** clarify roleNotValid message ([#4374](#4374)) ([0f8a9af](0f8a9af)) - **scrollable-region-focusable:** missing wcag213 tag ([#4201](#4201)) ([0080a72](0080a72)) - **target-size:** always pass 10x targets (avoid perf bottleneck) ([#4376](#4376)) ([be327c4](be327c4)) - **target-size:** do not crash for nodes with many overlapping widgets ([#4373](#4373)) ([1dbea83](1dbea83)), closes [#4359](#4359) [#4359](#4359) [#4360](#4360) - **utils/get-selector:** ignore 'xmlns' attribute when generating a selector ([#4303](#4303)) ([938b411](938b411)) This PR was opened by a robot 🤖 🎉
Decided that instead of returning
null
,splitRects
should return an empty array to signify the bail out state.splitRects
returns the original node if there is no overlap, so an empty array allows us to still signify the desired state as well as still allow all the code that used it to treat it as an array. Otherwise I would need to propagate anull
check through 4 different functions intarget-offset-evaluate
(getOffset
,getTargetSize
,getTargetRects
, etc.).In trying to catch the new state in
target-size-evaluate
it became difficult to figure out the logic of all the different things that needed to be checked to know what to return. I decided to refactor the entire function to make the flow easier by eliminating possibilities higher up so the if statements only needed to check a single condition rather than multiple conditions. The two key parts of the refactor was to move the overflow content check to the top.For overflowing content the return would always be undefined unless the target had sufficient size, in which case it would return true. This meant we can check those states first and not have to check for it again, which greatly simplifies all the logic of the if statements.
In terms of the magic number for when to exit early, it was based on running performance tests against the code in #4359 and logging how long the inner loop took as
uniqueRects
size grew. Once the time got to ~2 seconds to complete I took that number then reduced it by a factor of 10x for slower machines.Closes: #4359
Closes: #4360