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(create-grid): correctly compute stack order for non-positioned stacking contexts #3930

Merged
merged 8 commits into from
Mar 15, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Mar 3, 2023

While working on the code, I realized that the spec said that if any element that creates a stacking context it would remove the fake stacks created from float or positioned elements that use auto or 0 z-index. So I was able to merge those functions into a single if statement. I also simplified the loop to remove fake stacks so it only needs to look through the array and splice once (since any new stack removes all previous fake stacks).

Closes: #3929

@straker straker requested a review from a team as a code owner March 3, 2023 00:52
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

  1. I tried to refactor this function a bit, in order to help myself understand it better, please use that: https://github.com/dequelabs/axe-core/pull/3932/files

  2. I think we should move getStackingOrder into its own file. It's only tangentially related to creating the grid. I think separating these two could make things a little clearer.

Overall, I can't seem to keep this all straight in my head. I hope I didn't miss anything, but I'm not sure.

test/commons/dom/get-element-stack.js Show resolved Hide resolved
lib/commons/dom/create-grid.js Outdated Show resolved Hide resolved
lib/commons/dom/create-grid.js Outdated Show resolved Hide resolved
* chore: Refactor createStackingOrder

* Remove magic numbers
@straker
Copy link
Contributor Author

straker commented Mar 6, 2023

I think we should move getStackingOrder into its own file. It's only tangentially related to creating the grid. I think separating these two could make things a little clearer.

I don't think we should. The reason being that if we did we'd have to add tests for it, which means testing stacking context arrays. Those would be a pain to maintain as doing something like we just did (refactoring magic numbers, adding a new stack where one wasn't before) would break all the tests. I'd much rather test actual stack results like we are doing in getElementStack since those aren't fragile and won't break when we update stacking numbers.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Would have preferred to see createStackingOrder move into its own file, but

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.

Color-contrast issue with element stack
2 participants