Skip to content

fix(color-contrast): correctly compute background color for elements with opacity#3944

Merged
straker merged 16 commits intodevelopfrom
color-contrast-opacity
Mar 22, 2023
Merged

fix(color-contrast): correctly compute background color for elements with opacity#3944
straker merged 16 commits intodevelopfrom
color-contrast-opacity

Conversation

@straker
Copy link
Copy Markdown
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
Copy Markdown
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.

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
Copy Markdown
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
Copy Markdown
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.


const stackingContext = [];
const contextMap = new Map();
elmStack = elmStack ?? getBackgroundStack(elm);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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