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

feat(utils): add getFrameContexts method #2995

Merged
merged 9 commits into from
Jun 24, 2021
Merged

Conversation

WilcoFiers
Copy link
Contributor

Add a utility which is passed a context object, and returns the context for each iframe.

Closes issue: #2937

import getAncestry from './get-ancestry'

export default function getFrameContexts(context) {
const { frames } = new Context(context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates axe._tree, which is going to slow stuff down. Can we avoid that? And if not, how do we avoid either a tree that is outdated by the time axe.runPartial is called, or generating the tree twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to fix this in this pr or another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, we can let Context build axe._tree. I've accounted for it in the updated proposal.

@straker straker marked this pull request as ready for review June 22, 2021 17:20
@straker straker requested a review from a team as a code owner June 22, 2021 17:20
@WilcoFiers WilcoFiers marked this pull request as draft June 23, 2021 09:49
lib/core/base/context.js Outdated Show resolved Hide resolved
lib/core/public/load.js Outdated Show resolved Hide resolved
import getAncestry from './get-ancestry'

export default function getFrameContexts(context) {
const { frames } = new Context(context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, we can let Context build axe._tree. I've accounted for it in the updated proposal.

test/core/utils/get-frame-context.js Show resolved Hide resolved
test/core/utils/get-frame-context.js Show resolved Hide resolved
test/testframe1.html Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers marked this pull request as ready for review June 23, 2021 09:49
Copy link
Contributor Author

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@WilcoFiers
Copy link
Contributor Author

For the record: I passed this one off to Steve, who finished it up. Steve checked my work, and I checked his additions. Michael reviewed this for security.

@WilcoFiers WilcoFiers merged commit f478bab into develop Jun 24, 2021
@WilcoFiers WilcoFiers deleted the get-frame-contexts branch June 24, 2021 10:43
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