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(color-contrast): correctly handle flex and position #4086

Merged
merged 4 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 36 additions & 40 deletions lib/commons/dom/create-grid.js
WilcoFiers marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import assert from '../../core/utils/assert';
const ROOT_ORDER = 0;
const DEFAULT_ORDER = 0.1;
const FLOAT_ORDER = 0.2;
const POSITION_STATIC_ORDER = 0.3;
const POSITION_ORDER = 0.3;
let nodeIndex = 0;

/**
Expand Down Expand Up @@ -39,7 +39,7 @@ export default function createGrid(
}

nodeIndex = 0;
vNode._stackingOrder = [createContext(ROOT_ORDER, null)];
vNode._stackingOrder = [createContext(ROOT_ORDER, null, nodeIndex++)];
rootGrid ??= new Grid();
addNodeToGrid(rootGrid, vNode);

Expand Down Expand Up @@ -254,65 +254,61 @@ function isFlexOrGridContainer(vNode) {
* zIndex values for each stacking context parent.
* @param {VirtualNode} vNode
* @param {VirtualNode} parentVNode
* @param {Number} nodeIndex
* @param {Number} index
straker marked this conversation as resolved.
Show resolved Hide resolved
* @return {Number[]}
*/
function createStackingOrder(vNode, parentVNode, nodeIndex) {
function createStackingOrder(vNode, parentVNode, index) {
straker marked this conversation as resolved.
Show resolved Hide resolved
const stackingOrder = parentVNode._stackingOrder.slice();

// if a positioned element has z-index: auto or 0 (step #8), or if
// a non-positioned floating element (step #5), treat it as its
// own stacking context
// @see https://www.w3.org/Style/css2-updates/css2/zindex.html
if (!isStackingContext(vNode, parentVNode)) {
if (vNode.getComputedStylePropertyValue('position') !== 'static') {
// Put positioned elements above floated elements
stackingOrder.push(createContext(POSITION_STATIC_ORDER, vNode));
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

} else if (vNode.getComputedStylePropertyValue('float') !== 'none') {
// Put floated elements above z-index: 0
// (step #5 floating get sorted below step #8 positioned)
stackingOrder.push(createContext(FLOAT_ORDER, vNode));
}
return stackingOrder;
}

// if an element creates a stacking context, find the first
// true stack (not a "fake" stack created from positioned or
// floated elements without a z-index) and create a new stack at
// that point (step #5 and step #8)
// @see https://www.w3.org/Style/css2-updates/css2/zindex.html
const index = stackingOrder.findIndex(({ value }) =>
[ROOT_ORDER, FLOAT_ORDER, POSITION_STATIC_ORDER].includes(value)
);
if (index !== -1) {
stackingOrder.splice(index, stackingOrder.length - index);
if (isStackingContext(vNode, parentVNode)) {
const index = stackingOrder.findIndex(({ value }) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't have two variables with the same name in one function. This code is complicated enough 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please

[ROOT_ORDER, FLOAT_ORDER, POSITION_ORDER].includes(value)
);
if (index !== -1) {
stackingOrder.splice(index, stackingOrder.length - index);
}
}

const zIndex = getRealZIndex(vNode, parentVNode);
if (!['auto', '0'].includes(zIndex)) {
stackingOrder.push(createContext(parseInt(zIndex), vNode));
stackingOrder.push(createContext(parseInt(zIndex), vNode, index));
return stackingOrder;
}

// if a positioned element has z-index: auto or 0 (step #8), or if
// a non-positioned floating element (step #5), treat it as its
// own stacking context
// @see https://www.w3.org/Style/css2-updates/css2/zindex.html
if (vNode.getComputedStylePropertyValue('position') !== 'static') {
// Put positioned elements above floated elements
stackingOrder.push(createContext(POSITION_ORDER, vNode, index));
return stackingOrder;
}

if (vNode.getComputedStylePropertyValue('float') !== 'none') {
// Put floated elements above z-index: 0
// (step #5 floating get sorted below step #8 positioned)
stackingOrder.push(createContext(FLOAT_ORDER, vNode, index));
return stackingOrder;
}

if (!isStackingContext(vNode, parentVNode)) {
return stackingOrder;
}
// since many things can create a new stacking context without position or
Copy link
Contributor

@dbjorge dbjorge Jul 11, 2023

Choose a reason for hiding this comment

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

This is a nice change, I think your update including the nodeIndex in the comparator is much cleaner than the old way.

// z-index, we need to know the order in the dom to sort them by. Use the
// nodeIndex property to create a number less than the "fake" stacks from
// positioned or floated elements but still larger than 0
// 10 pad gives us the ability to sort up to 1B nodes (padStart does not
// exist in ie11)
let float = nodeIndex.toString();
while (float.length < 10) {
float = '0' + float;
}
stackingOrder.push(
createContext(parseFloat(`${DEFAULT_ORDER}${float}`), vNode)
);

stackingOrder.push(createContext(DEFAULT_ORDER, vNode, index));
return stackingOrder;
}

function createContext(value, vNode) {
function createContext(value, vNode, index) {
return {
value,
straker marked this conversation as resolved.
Show resolved Hide resolved
nodeIndex: index,
straker marked this conversation as resolved.
Show resolved Hide resolved
straker marked this conversation as resolved.
Show resolved Hide resolved
vNode
};
}
Expand Down
5 changes: 5 additions & 0 deletions lib/commons/dom/visually-sort.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ export default function visuallySort(a, b) {
if (b._stackingOrder[i].value < a._stackingOrder[i].value) {
return -1;
}

// stacks share the same stacking context value so compare document order
if (b._stackingOrder[i].nodeIndex !== a._stackingOrder[i].nodeIndex) {
return b._stackingOrder[i].nodeIndex - a._stackingOrder[i].nodeIndex;
straker marked this conversation as resolved.
Show resolved Hide resolved
}
}

// nodes are the same stacking order
Expand Down
22 changes: 22 additions & 0 deletions test/commons/dom/get-element-stack.js
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,28 @@ describe('dom.getElementStack', () => {
assert.deepEqual(stack, []);
});

it('should correctly position children of different stacking contexts', () => {
fixture.innerHTML = `
<header id="1" style="position: absolute; z-index: 999; height: 50px; width: 100%; top: 0">
<div id="2" style="display: flex; position: relative; height: 50px;">
<div id="3" style="position: relative; height: 50px; width: 100%;"></div>
</div>
<div id="4" style="display: flex; position: absolute; height: 50px; width: 100%; top: 0;">
<div id="5" style="position: absolute; transform: translate(0, -50%);">
<div id="6" style="display: flex;">
<span id="target">Hello World</span>
</div>
</div>
</div>
</header>
`;

axe.testUtils.flatTreeSetup(fixture);
const target = fixture.querySelector('#target');
const stack = mapToIDs(getElementStack(target));
assert.deepEqual(stack, ['target', '6', '5', '4', '3', '2', '1']);
});

it('should throw error if element midpoint-x exceeds the grid', () => {
fixture.innerHTML = '<div id="target">Hello World</div>';
axe.testUtils.flatTreeSetup(fixture);
Expand Down