Skip to content
This repository has been archived by the owner on Mar 19, 2021. It is now read-only.

fix: Improve error message when an iframe was removed #118

Merged
merged 3 commits into from
Jul 9, 2019

Conversation

AdnoC
Copy link
Contributor

@AdnoC AdnoC commented Jul 8, 2019

After trying many things, this is the best we can do. When a StaleElementReferenceError we have a reference to an element that was either deleted or detached from the DOM. And we shouldn't be doing anything that could cause either of those things to occur. If a user's page causes Selenium to throw a StaleElementReferenceError then we our Axe results probably won't be consistent anyways.

So, the best solution is to inform the user that it is their page that is acting up and that it is their responsibility to get the page in a stable state before running Axe on it.

Closes issue: https://github.com/dequelabs/attest-node-suite/issues/246

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: Stephen

@AdnoC AdnoC requested a review from WilcoFiers as a code owner July 8, 2019 16:02
@CLAassistant
Copy link

CLAassistant commented Jul 8, 2019

CLA assistant check
All committers have signed the CLA.

@AdnoC AdnoC force-pushed the fix-lloyds-err-msg branch from cd00357 to 71560c2 Compare July 8, 2019 16:04
console.error('Failed to inject axe-core into one of the iframes!');
if (err instanceof StaleElementReferenceError) {
// eslint-disable-next-line no-console
console.error('Tried to inject into a removed iframe. Please ensure the page has settled.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to: Tried to inject into a removed iframe. This will not affect the analysis of the rest of the page but you might want to ensure the page has finished updating before starting the analysis

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Requesting changes per Dylan's comment.

@AdnoC AdnoC dismissed stephenmathieson’s stale review July 9, 2019 15:23

Changed the message

@AdnoC AdnoC merged commit d625312 into develop Jul 9, 2019
@AdnoC AdnoC deleted the fix-lloyds-err-msg branch July 9, 2019 16:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants