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 compute background color for elements with opacity #3944

Merged
merged 16 commits into from
Mar 22, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Mar 14, 2023

This is the 2nd part of fixing #3924. With this I have the ability to grab the stacking context for the foreground element and then compute the separate background colors where the opacity is applied so I can compute the text color to the background behind the text's element stack.

@straker straker requested a review from a team as a code owner March 14, 2023 16:57
// Search the stack until we have an alpha === 1 background
(elmStack || []).some(bgElm => {
for (let i = 0; i < elmStack.length; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By turning this into a loop instead of a some we can return early inside the loop rather than trying to return after the some has already gone through all elements of the stack.

@@ -267,11 +267,11 @@ function createStackingOrder(vNode, parentVNode, nodeIndex) {
if (!isStackingContext(vNode, parentVNode)) {
if (vNode.getComputedStylePropertyValue('position') !== 'static') {
// Put positioned elements above floated elements
stackingOrder.push(POSITION_STATIC_ORDER);
stackingOrder.push(createContext(POSITION_STATIC_ORDER, vNode));
Copy link
Contributor Author

@straker straker Mar 15, 2023

Choose a reason for hiding this comment

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

Adding virtual node info to the stacking order allows me to use that virtual node to create the stacking context object for the node.

})
.concat(alpha);
}
const hexRegex = /^#[0-9a-f]{3,8}$/i;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this opportunity to do something I've been wanting to do for awhile now which is convert the Color constructor function into a true class. The advantage to this was that I was able to do deepEquals on two Color objects whereas before that was not possible (thus greatly simplifying the tests). Another advantage is that all Color objects now share memory for all their functions thanks to the prototype whereas before each Color object had their own memory instance for each function.

lib/commons/color/stacking-context.js Outdated Show resolved Hide resolved

const stackingContext = [];
const contextMap = new Map();
elmStack = elmStack ?? getBackgroundStack(elm);
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this ever not be defined? All calls I can see pass something in. Possibly an empty array, but not null or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make it optional since the foreground color check will use this function to apply opacity but not call the elmStack itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll want to do some performance testing of getBackgroundStack to see if it's an expensive call that warrants caching the result for when foreground color needs it.

Copy link
Contributor Author

@straker straker Mar 22, 2023

Choose a reason for hiding this comment

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

Ran some performance tests using the following code:

performance.clearMarks();
performance.clearMeasures();
performance.mark('start');
stack = axe.commons.color.getBackgroundStack($0);
performance.mark('end');
performance.measure('time', 'start', 'end');
console.log(stack, performance.getEntriesByName('time')[0].duration)

On https://js.tensorflow.org/api/latest/, the first call to color.getBackgroundStack() takes < 1ms, with subsequent calls taking < 0.2ms.

screenshot of running multiple performance tests on tensorflow showing times of .600ms, .199ms, and .200ms

On another page (https://api.freshservice.com/v2/#view_all_vendors), the results were similar with the first call taking 2ms while subsequent calls only took < 0.6ms.

screenshot of running multiple performance tests on api.freshservice showing times of 2.3ms, .600ms, and .299ms

Note that these tests were done after the grid was created so that grid creation didn't throw off the first call.

I also tried calling it on different elements then returning to the first to see if that impacted the time, but it showed similar results.

function randInt(min, max) {
  return ((Math.random() * (max - min + 1)) | 0) + min;
}

const node = $0;
const elms = document.querySelectorAll('*');
for (let i = 0; i < 100; i++) {
    const elm = elms[ randInt(0, elms.length - 1) ];
    const stack = axe.commons.color.getBackgroundStack(elm);
}

performance.clearMarks();
performance.clearMeasures();
performance.mark('start');
stack = axe.commons.color.getBackgroundStack(node);
performance.mark('end');
performance.measure('time', 'start', 'end');
console.log(stack, performance.getEntriesByName('time')[0].duration)  // 0.200

test/commons/color/stacking-context.js Show resolved Hide resolved
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.

2 participants