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

Bump puppeteer from 19.3.0 to 19.4.0 #3084

Merged
merged 3 commits into from Dec 12, 2022

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Dec 8, 2022

Bumps puppeteer from 19.3.0 to 19.4.0.

Release notes

Sourced from puppeteer's releases.

puppeteer-core: v19.4.0

19.4.0 (2022-12-07)

Features

  • ability to send headers via ws connection to browser in node.js environment (#9314) (937fffa), closes #7218
  • chromium: roll to Chromium 109.0.5412.0 (r1069273) (#9364) (1875da6), closes #9233
  • puppeteer-core: keydown supports commands (#9357) (b7ebc5d)

Bug Fixes

puppeteer: v19.4.0

19.4.0 (2022-12-07)

Features

Dependencies

  • The following workspace dependencies were updated
    • dependencies
      • puppeteer-core bumped from 19.3.0 to 19.4.0
Commits
  • 931d4fc chore: release main (#9322)
  • 1875da6 feat(chromium): roll to Chromium 109.0.5412.0 (r1069273) (#9364)
  • 1501ede docs(requestinterception): remove outdated note (#9358)
  • 17f31a9 fix(puppeteer-core): avoid type instantiation errors (#9370)
  • be7626b chore: fix race in test Frame.waitForFunction should work when context is des...
  • 51d75a0 chore(ng-schematics): Use WireIt for builds and tests (#9356)
  • b7ebc5d feat(puppeteer-core): keydown supports commands (#9357)
  • 3cdd5d8 chore: fix build deps (#9344)
  • f3c87dc chore: upgrade mitt (#9340)
  • e8c1d56 chore: import BiDi impl only if the user opts in (#9335)
  • Additional commits viewable in compare view

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

@dependabot dependabot bot added dependencies Pull requests that update a dependency file javascript labels Dec 8, 2022
@dependabot dependabot bot requested a review from a team December 8, 2022 09:01
@colinrotherham
Copy link
Contributor

colinrotherham commented Dec 8, 2022

Looking into this one 👀

There's a Chromium update included but .find() is called on undefined when looping accessibility child nodes
https://github.com/puppeteer/puppeteer/blob/931d4fced52aefbefdc8b21edfe3d1528e4e448b/packages/puppeteer-core/src/common/Accessibility.ts#L319

Only happens after the first $element.click()

Seems to indicate either garbage collection (child nodes removed due to page navigation) or Chromium correctly holding a reference to $element as the original <span> with class .govuk-accordion__section-button not the <button>

  console.log
    TypeError: Cannot read properties of undefined (reading 'find')
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at AXNode.find (node_modules/puppeteer-core/src/common/Accessibility.ts:319:28)
        at Accessibility.snapshot (node_modules/puppeteer-core/src/common/Accessibility.ts:196:28)
        at processTicksAndRejections (node:internal/process/task_queues:95:5)
        at getAccessibleName (lib/puppeteer-helpers.js:147:16)
        at Object.<anonymous> (src/govuk/components/accordion/accordion.test.js:262:11)

Update: It was the <button> as the root element so that didn't explain the missing accessibility child nodes

await page.accessibility.snapshot({
  root: $element
})

dependabot bot and others added 2 commits December 8, 2022 11:48
Bumps [puppeteer](https://github.com/puppeteer/puppeteer) from 19.3.0 to 19.4.0.
- [Release notes](https://github.com/puppeteer/puppeteer/releases)
- [Changelog](https://github.com/puppeteer/puppeteer/blob/main/release-please-config.json)
- [Commits](puppeteer/puppeteer@puppeteer-v19.3.0...puppeteer-v19.4.0)

---
updated-dependencies:
- dependency-name: puppeteer
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@colinrotherham colinrotherham force-pushed the dependabot/npm_and_yarn/puppeteer-19.4.0 branch from e1d8a88 to 158467d Compare December 8, 2022 11:50
@colinrotherham
Copy link
Contributor

Just to highlight the Chromium changes

Looks like there are some internal (to Chromium) changes when aria-expanded state changes, but adding a 100ms delay from Accordion button .click() to calling page.accessibility.snapshot() fixes the child node loop ✅

Lots of commits landing so may not need this workaround in future:

Another one ongoing to stablise accessibility objects:
https://chromium-review.googlesource.com/c/chromium/src/+/4027071

Looks like there are some internal (to Chromium) changes when `aria-expanded` state changes, but adding a 100ms delay from Accordion button `.click()` to calling `page.accessibility.snapshot()` fixes the child node loop ✅

Lots of commits landing so may not need this workaround in future:

* chromium/chromium@08fed96
* chromium/chromium@c7fae74
* chromium/chromium@06f376b
* chromium/chromium@86543a2

Another one ongoing to stablise accessibility objects:
https://chromium-review.googlesource.com/c/chromium/src/+/4027071
@colinrotherham colinrotherham force-pushed the dependabot/npm_and_yarn/puppeteer-19.4.0 branch from 158467d to af91aae Compare December 8, 2022 11:56
* @param {import('puppeteer').ElementHandle} $element - Puppeteer element handle
* @returns {string} The element's accessible name
* @throws {TypeError} If the element has no corresponding node in the accessibility tree
*/
async function getAccessibleName ($element) {
async function getAccessibleName (page, $element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've seen Jest Puppeteer replace the global page between concurrent tests so now a param

@@ -251,13 +251,14 @@ describe('/components/accordion', () => {

const $element = await page.$('.govuk-accordion__section-button')

expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Puppeteer was receiving the .click() that follows concurrent to Jest resolving this promise

It was flagged by TypeScript in another PR so was my first guess at fixing the issue. We now wait for the assertion to complete before the Accordion button gets a .click()

Copy link
Member

Choose a reason for hiding this comment

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

Neat! Thanks for fixing that 🙌🏻

'Section A , Show this section'
)

await $element.click()
await page.waitForTimeout(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

We see child is undefined in src/common/Accessibility.ts#L319 unless a short delay is added

For a single test run 50ms is sufficient but still saw occasional failures. As mentioned in the comments, this is possibly due to accessibility tree (and text announcement) changes when aria-expanded is updated

Copy link
Member

Choose a reason for hiding this comment

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

suggestion Rather than a timeout, we could use an aria selector to make Puppeteer wait. Once we're past that lookup, we know the accessibility tree has been updated and we can run our check that it's the element we hold a reference of that's actually been updated.

Suggested change
await page.waitForTimeout(100)
// Puppeteer doesn't update the accessibility tree in time for our next expectation
// So we use an aria selector to wait for the accessibility tree to be ready
// before running our actual check.
await page.$('aria/Section A , Hide this section[role="button"]')

Unfortunately Puppeteer still doesn't expose the necessary APIs to compare that this page.$ references the same node as $element (though looks like they're making progress).

Copy link
Member

Choose a reason for hiding this comment

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

Thinking we could encapsulate that inside the getAccessibleName() helper with a {wait: true} option that would save typing the accessible name twice and keep things neatly encapsulated 😄

Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Approving the update, but might need @domoscargin's capable brain on this one too

@colinrotherham colinrotherham added this to Needs review 🔍 in Design System Sprint Board via automation Dec 8, 2022
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Cheers for digging into this one! Suggested an alternative route that would help us avoid timeouts, as they are a risk of flakiness for these kinds of fixes in my experience 😄

'Section A , Show this section'
)

await $element.click()
await page.waitForTimeout(100)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion Rather than a timeout, we could use an aria selector to make Puppeteer wait. Once we're past that lookup, we know the accessibility tree has been updated and we can run our check that it's the element we hold a reference of that's actually been updated.

Suggested change
await page.waitForTimeout(100)
// Puppeteer doesn't update the accessibility tree in time for our next expectation
// So we use an aria selector to wait for the accessibility tree to be ready
// before running our actual check.
await page.$('aria/Section A , Hide this section[role="button"]')

Unfortunately Puppeteer still doesn't expose the necessary APIs to compare that this page.$ references the same node as $element (though looks like they're making progress).

'Test , Additional description , Show this section'
)

await $element.click()
await page.waitForTimeout(100)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await page.waitForTimeout(100)
await page.$('aria/Test , Additional description , Hide this section[role="button"]')

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah don't worry, like you said we're back to not knowing if $element is the same "element" initially clicked

We may well see puppeteer-core include the delay at their end should it be an intentional Chromium change

I'm happy with this workaround temporarily

Copy link
Member

Choose a reason for hiding this comment

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

@@ -251,13 +251,14 @@ describe('/components/accordion', () => {

const $element = await page.$('.govuk-accordion__section-button')

expect(getAccessibleName($element)).resolves.toBe(
await expect(getAccessibleName(page, $element)).resolves.toBe(
Copy link
Member

Choose a reason for hiding this comment

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

Neat! Thanks for fixing that 🙌🏻

@colinrotherham colinrotherham merged commit 599abf6 into main Dec 12, 2022
Design System Sprint Board automation moved this from Needs review 🔍 to Done 🏁 Dec 12, 2022
@colinrotherham colinrotherham deleted the dependabot/npm_and_yarn/puppeteer-19.4.0 branch December 12, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file javascript
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants