Skip to content
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

ContextObject type definition does not match actual context object #3793

Closed
WilcoFiers opened this issue Nov 22, 2022 · 1 comment · Fixed by #3798
Closed

ContextObject type definition does not match actual context object #3793

WilcoFiers opened this issue Nov 22, 2022 · 1 comment · Fixed by #3798
Assignees
Labels
core Issues in the core code (lib/core) fix Bug fixes
Milestone

Comments

@WilcoFiers
Copy link
Contributor

WilcoFiers commented Nov 22, 2022

Our ContextObject is defined as follows:

type BaseSelector = string;
type ContextObject = {
  include?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>;
  exclude?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>;
};

At least two of those are impossible today:

// This does not work at all. This tests the entire page:
axe.run({ include: 'main' })
// Oddly enough, this only tests `header`.
// Everything after the first string is ignored:
axe.run({ include: ['header', 'main'] })

Neither of these options is described in our docs. That makes me thing these were never supposed to work, and we created an error in the type definition. That said, we have tons of tests that do something like {include: ['#fixture']}, presumably others building on top of axe have done similar things. I don't really know why these do work. Going from the code it looks intentional. And to allow multiple strings in that array, the only change would be to change a break in a for-loop to a continue. That seems like a bug.

The way I see it, we can either change our types to what we actually support, or update what we support to match the types. I'm leaning towards the latter. It adds a little more complexity to Context, but makes them more intuitive in my opinion. If we didn't want that complexity we probably stop supporting {include: ['#fixture']}, which I'm concerned may break other implementations.

@WilcoFiers WilcoFiers added fix Bug fixes core Issues in the core code (lib/core) labels Nov 22, 2022
@WilcoFiers WilcoFiers changed the title Context should allow multiple strings ContextObject type definition does not match actual context object Nov 23, 2022
@WilcoFiers WilcoFiers self-assigned this Nov 28, 2022
@WilcoFiers WilcoFiers added this to the Axe-core 4.6 milestone Nov 28, 2022
@WilcoFiers WilcoFiers modified the milestones: Axe-core 4.7, axe-core 4.6 Nov 30, 2022
@padmavemulapati
Copy link

padmavemulapati commented Dec 5, 2022

Validated with the latest code base of axe-core develop branch,

Tested on test page http://qateam.dequecloud.com/attest/api/test.html
Now if we run await axe.run({ include: ['main'] }); we can see the context object with the expected results

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues in the core code (lib/core) fix Bug fixes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants