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

chore(origin): fix CDP Page.setDocumentContent origin null crashing a… #3912

Closed
wants to merge 1 commit into from
Closed

Conversation

j-mendez
Copy link
Contributor

  • fix issue with test hanging on allowedOrigins unhandled exception when using CDP Page.setDocumentContent

--- Notes

The following issue occurs when using axe-core to test HTML markup created over CDP Page.setDocumentContent or puppeteer.setContent.

[0213/121447.712165:INFO:CONSOLE(15)] "Uncaught (in promise) Error: allowedOrigins value "null" is not a valid origin", source: pptr://__puppeteer_evaluation_script__ (15)

Example test showing the crash repro https://github.com/a11ywatch/a11y/blob/main/tests/basic-axe.ts.
You also need to comment out the following block https://github.com/a11ywatch/a11y/blob/main/lib/runners/axe.ts#L16 that allowing test to run after setting the origin manually.

@j-mendez j-mendez requested a review from a team as a code owner February 13, 2023 17:20
@CLAassistant
Copy link

CLAassistant commented Feb 13, 2023

CLA assistant check
All committers have signed the CLA.

@j-mendez j-mendez closed this by deleting the head repository Feb 15, 2023
@straker
Copy link
Contributor

straker commented Feb 15, 2023

@j-mendez did you mean to close this? I was looking into this pr and was able to validate the bug and the fix. Was just about to ask for a test if that was possible (was seeing if it was possible to change window.origin and window.locaiton.origin to 'null' in a test env without using CDP)

@j-mendez
Copy link
Contributor Author

@straker Hi, I actually did not mean to close down the PR. Changing the 'null' for a test seems adequate as well. Is there a certain location or file for the test additions?

@j-mendez j-mendez reopened this Feb 15, 2023
@straker
Copy link
Contributor

straker commented Feb 16, 2023

Alight, talked with @WilcoFiers on how best to test this. What we think we'd like is a new example in the docs/examples/puppeteer directory that uses puppeteers page.setContent function to create the page with an iframe. Then trying to run axe on that page should work (meaning you get results back without an error). That should allow the code to be tested and prevent regressions.

Something like this (you'll need to update the example pacakge.json test script to also run the new file).

// doc/examples/puppeteer/set-content.js
const assert = require('assert');

const browser = await puppeteer.launch();
const page = await browser.newPage();
const axeSource = await fs.readFile('./axe.js', 'utf8');

await page.setContent(`
  <html lang="en">
  <head>
    <title>Test Page</title>
  </head>
  <body>
    <main>
      <h1>Hello World</h1>
      <button id="empty-button"></button>
      <iframe title="iframe" srcdoc="<button id='iframe-empty-button'></button>"></iframe>
    </main>
  </body>
  </html>
`);

await page.evaluate(axeSource);
const frames = page.frames();
for (let i = 0; i < frames.length; i++) {
  await frames[i].evaluate(axeSource);
}

const results = await page.evaluate(`window.axe.run()`);
assert(results.violations.length)

@j-mendez
Copy link
Contributor Author

@straker feel free to drive this one home, currently do not have the bandwidth.

@straker
Copy link
Contributor

straker commented Feb 22, 2023

Looks like you deleted your fork that this pr was attached to. I'll have to close this pr and open a new one to make the changes.

@j-mendez
Copy link
Contributor Author

j-mendez commented Feb 22, 2023

@straker you can use the GitHub cli to checkout the pr or git https://cli.github.com/manual/gh_pr_checkout.

@straker
Copy link
Contributor

straker commented Feb 22, 2023

It's not possible to checkout your code through the CLI as it no longer exists

image

@straker
Copy link
Contributor

straker commented Feb 22, 2023

Wo, didn't know the gh command could do that. Thanks for the reference.

@j-mendez
Copy link
Contributor Author

Wo, didn't know the gh command could do that. Thanks for the reference.

no problem, it should be gh pr checkout 3912 for this PR.

@straker
Copy link
Contributor

straker commented Feb 22, 2023

So now that I've made the changes, how do I push back to this pr? I don't see a push command for prs in gh, and there's not a branch for this pr techincally.

@j-mendez
Copy link
Contributor Author

So now that I've made the changes, how do I push back to this pr? I don't see a push command for prs in gh, and there's not a branch for this pr techincally.

I think ‘gh pr push’, not sure only since I have only recently used it to checkout a PR for testing.

@straker
Copy link
Contributor

straker commented Feb 22, 2023

Hmm, didn't work. This is an interesting problem.

@j-mendez
Copy link
Contributor Author

Hmm, didn't work. This is an interesting problem.

Ahh, I’m not at the computer right now. Feel free to merge the changes locally to another branch and re push the PR or etc and thank you for taking it over!

@straker
Copy link
Contributor

straker commented Feb 22, 2023

Moved pr to #3921.

@straker straker closed this Feb 22, 2023
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