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

fix(heading-order): Crash on page with iframes but no headings #2965

Merged
merged 7 commits into from
Jun 2, 2021

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented May 29, 2021

This PR is a rewrite of heading-order-after. It fixes a number of issues:

Closes issue: #2928 and #2944

@WilcoFiers WilcoFiers force-pushed the heading-order-after-throws branch 2 times, most recently from e51827f to d24ce27 Compare May 31, 2021 15:01
@WilcoFiers WilcoFiers requested a review from straker May 31, 2021 15:17
@WilcoFiers WilcoFiers marked this pull request as ready for review May 31, 2021 15:17
@WilcoFiers WilcoFiers requested a review from a team as a code owner May 31, 2021 15:17
@gdkraus
Copy link

gdkraus commented Jun 1, 2021

@WilcoFiers, in reference to #2944 the description of what this change solves does not describe our situation. In our app, we have an iframe that has multiple headings in it.

@gdkraus
Copy link

gdkraus commented Jun 1, 2021

@WilcoFiers, or do you mean if the outermost frame contains no headings, but contains iframes, that is when the problem occurs?

@gdkraus
Copy link

gdkraus commented Jun 1, 2021

I manually edited our parent frame to have a single heading and ran axe DevTools again and it worked.

@straker straker merged commit 99e7f0c into develop Jun 2, 2021
@straker straker deleted the heading-order-after-throws branch June 2, 2021 16:24
straker pushed a commit that referenced this pull request Jun 3, 2021
#2965)

* fix(heading-order): Crash on page with iframes but no headings

* chore: Fix linter issues

* chore: fewer things undefined

* chore: attempt to fix IE stuff

* chore: refactor

* chore: tweak heading-order test

* fix: heading-order properly filter placeholders
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