fix(target-size): do not crash for nodes with many overlapping widgets #4373
Conversation
This file contains 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
WilcoFiers
previously requested changes
Mar 15, 2024
| @@ -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.
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.
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>
WilcoFiers
requested changes
Mar 18, 2024
WilcoFiers
approved these changes
Mar 18, 2024
Merged
WilcoFiers
added a commit
that referenced
this pull request
Mar 25, 2024
## [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 🤖 🎉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Decided that instead of returning
null,splitRectsshould return an empty array to signify the bail out state.splitRectsreturns 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 anullcheck through 4 different functions intarget-offset-evaluate(getOffset,getTargetSize,getTargetRects, etc.).In trying to catch the new state in
target-size-evaluateit 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
uniqueRectssize 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