fix(color-contrast): correctly handle flex and position #4086
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbjorge
reviewed
Jul 11, 2023
dbjorge
reviewed
Jul 11, 2023
| return stackingOrder; | ||
| } | ||
| // since many things can create a new stacking context without position or |
There was a problem hiding this comment.
This is a nice change, I think your update including the nodeIndex in the comparator is much cleaner than the old way.
WilcoFiers
previously requested changes
Jul 12, 2023
lib/commons/dom/create-grid.js
Outdated
| if (index !== -1) { | ||
| stackingOrder.splice(index, stackingOrder.length - index); | ||
| if (isStackingContext(vNode, parentVNode)) { | ||
| const index = stackingOrder.findIndex(({ value }) => |
There was a problem hiding this comment.
Please don't have two variables with the same name in one function. This code is complicated enough 😁
There was a problem hiding this comment.
Thoughts on a separate PR enabling the no-shadow eslint rule to catch this sort of issue proactively? (111 pre-existing issues, not trivial but manageable)
| if (!isStackingContext(vNode, parentVNode)) { | ||
| if (vNode.getComputedStylePropertyValue('position') !== 'static') { | ||
| // Put positioned elements above floated elements | ||
| stackingOrder.push(createContext(POSITION_STATIC_ORDER, vNode)); |
There was a problem hiding this comment.
Wow, did we call the position for when something's NOT static the POSITION_STATIC_ORDER. Whoops! 🙄 That was my idea too wasn't it?
WilcoFiers
approved these changes
Jul 13, 2023
Comment on lines
+291
to
+293
| * @param {Number} stackLevel - The stack level of the stacking context | ||
| * @param {Number} treeOrder - The elements depth-first traversal order | ||
| * @param {VirtualNode} vNode - The virtual node that is the container for the stacking context |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly it's rearranging the order of the
createStackingContextfunction to handle elements that created stacking context and needed to be positioned or floated. The code didn't handle elements that were both positioned withz-index: autoand another property that made it a stacking context (likedisplay: flex).Also changed how to handle adding the node index to every context by making it it's own property instead of combining it with the stacking context value. Made things simpler when creating / comparing the numbers. This was needed to allow sorting elements from different (but equal) stacking contexts correctly. Before they would be sorted as equal (stacking contexts were the same value) and then relied on the document order and positional sorting (based on float or inline), even when they belonged to different stacking context and shouldn't be sorted by position. Now the elements will only be positionally sorted when they belong to the same stacking context.
It should be noted that this fix doesn't actually result in resolving the incomplete in #4041. There is another element (
.tmpl-header_searchBtnContainer) that is on top of the others (with a transparent background) and so the incomplete is correct that there is another element overlapping the links. I didn't realize this until after I made the stack correct.Closes: #4041