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.

Sign up for GitHub

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

Jump to bottom

Allow passing a Page instead of an ElementHandle to getAccessibilityTree #344

Merged
merged 6 commits into from
Jan 3, 2022
Merged

Allow passing a Page instead of an ElementHandle to getAccessibilityTree #344

merged 6 commits into from
Jan 3, 2022

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Dec 22, 2021 •
edited
Loading

Overview

After using getAccessibilityTree a bit I realized that it is very common to want to do getAccessibilityTree on the whole document, so I made a shortcut to get a reference to the body element:

const body = await screen.container

This is equivalent to:

const body = await page.evaluateHandle<ElementHandle>(() => document.body)

Also, it can be used to get a reference to the container element used after calling within(el) to get queries scoped to a specific element:

const elQueries = within(el)
const container = await elQueries.container

In this example, container and el are both ElementHandles that point to the same element.

Alternate solutions

  • Other names for .container
    • root
    • el
    • element
  • Make getAccessibilityTree default to the document if no element is passed 
    expect(await getAccessibilityTree(/* implied body */)).toMatchInlineSnapshot()
  • Add a parameter that is passed to the withBrowser callback 
    withBrowser(async ({ getBody }) => {
  const body = await getBody()
})

I don't know which of these solutions would be best (it is also possible to implement multiple of them). Let me know if y'all have thoughts on which would be best, it would be pretty easy to implement any of them.

Paul-Hebert
Paul-Hebert approved these changes Dec 23, 2021
View reviewed changes
Copy link
Contributor

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

This code looks good to me and it makes sense to have a quick way to grab the body.

I'm less clear on the use case of queryEl.container. In the examples you documented this variable is always matching an existing variable. Could you share more context for when you'd need this? It might be nice to add that context to the docs.

Make getAccessibilityTree default to the document if no element is passed

This alternative solution makes a lot of sense to me!

@calebeby
Copy link
Member Author

I'm less clear on the use case of queryEl.container ...

Me too 😂 . I mostly put it there so that it parallels the screen.container property since screen and within(body) are supposed to be the same thing.

Here is a somewhat contrived use case:

const heading = within(await screen.getByRole('heading'))

await heading.getByText('...') // etc

initHeading(heading.container);

Since the screen.getByRole('heading') query is passed directly in to within, it doesn't have its own variable, so the .container property is helpful here. But it could just as easily use a pair of variables, heading and headingEl to access the queries and the container.

Make getAccessibilityTree default to the document if no element is passed

Yeah I think this solution probably is better. I'll implement that and we can talk about removing .container

@Paul-Hebert
Copy link
Contributor

Sounds good 👍 If we end up leaving in queryEl.container we may want to update the docs to a use case similar to the one you mentioned above.

@calebeby calebeby requested a review from Paul-Hebert December 24, 2021 00:18
@calebeby
Copy link
Member Author

@Paul-Hebert I took out screen.container. Maybe we can re-add it if we have a good use case for it.

Now getAccessibilityTree accepts a Page as the first parameter, and if that is passed it will print the accessibility snapshot of the entire document (from the <html> element, not <body>).

Paul-Hebert
Paul-Hebert approved these changes Dec 28, 2021
View reviewed changes
Copy link
Contributor

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

LGTM!

@calebeby calebeby changed the title Add screen.container property Allow passing a page instead of an elementhandle to getAccessibilityTree Jan 3, 2022
@calebeby calebeby changed the title Allow passing a page instead of an elementhandle to getAccessibilityTree Allow passing a Page instead of an ElementHandle to getAccessibilityTree Jan 3, 2022
@calebeby calebeby merged commit d7bbae3 into main Jan 3, 2022
@calebeby calebeby deleted the screen-container branch January 3, 2022 21:33
@github-actions github-actions bot mentioned this pull request Jan 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Paul-Hebert Paul-Hebert Paul-Hebert approved these changes

@spaceninja spaceninja Awaiting requested review from spaceninja

@gerardo-rodriguez gerardo-rodriguez Awaiting requested review from gerardo-rodriguez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@calebeby @Paul-Hebert