Add tests for 2up and thumbnail view modes#210
Conversation
carylwyatt
left a comment
carylwyatt
left a comment
There was a problem hiding this comment.
I'm so happy to have additional tests in page turner! Thanks for all your work on this, it's really great! All of my comments have been a little pedantic, and I don't follow best practices all the time, so take it all with a grain of salt. Your tests have done a great job exposing parts of the markup that need improvement, so I'll add these to a list to chat about with Gayathri during UI review.
|
|
||
| test('shows a selectable button for each page', async ({ page }) => { | ||
| await page.goto('/cgi/pt?id=test.pd_open&view=thumb'); | ||
| await expect(page.locator('button[aria-label="Select scan #1"]')).toBeVisible(); |
There was a problem hiding this comment.
I can see that I used this locator previously in sidebar.spec.js so I'm about to contradict my own test, but I should've used getByRole('button', {name: 'Select scan #1'}). I need to go back and fix that.
These are kind of nitpicky and pedantic, but the testing philosophy of playwright is to test what the user will use, not underlying details like CSS class names. It's not incorrect or wrong to do that, but I like to to try to use the accessible names whenever possible, even though I obviously haven't kept to that in the past. Sometimes it's unavoidable because our markup isn't as accessible as it could be, like with the sidebar/complimentary role or the search results alert that doesn't have a role or name.
There was a problem hiding this comment.
It does not seem to work with getByRole. I think I know why because there is aria-hidden={!focused}.
scan #2 is always aria-hidden in this view, so it's invisible to getByRole which queries the accessibility tree.
And it means the element is absent from the accessibility tree, which is itself an a11y gap worth noting.
For now we might need to use CSS selectors to query the raw DOM - what do you think?
2 participants
Adds Playwright coverage for 2up and thumbnail views. The existing toolbar spec only checked URL changes when switching views, not that the right content actually rendered. These tests also cover loading views directly via URL param, which was completely untested.