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

feat: store getComputedStyle result to avoid redoing that work #1048

Merged
merged 9 commits into from
Jul 24, 2024
9 changes: 9 additions & 0 deletions .changeset/spotty-chicken-add.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"dom-accessibility-api": minor
---

Cache `window.getComputedStyle` results

Should improve performance in environments that don't cache these results natively e.g. JSDOM.
This increases memory usage.
If this results in adverse effects (e.g. resource constrained browser environments), please file an issue.
9 changes: 9 additions & 0 deletions sources/__tests__/accessible-name.js
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,15 @@ describe("options.getComputedStyle", () => {
expect(name).toEqual("foo test foo");
expect(window.getComputedStyle).not.toHaveBeenCalled();
});
it("is not called more than once per element", () => {
const container = renderIntoDocument(
"<button><span><span>nested</span>button</span></button>",
);

computeAccessibleName(container.querySelector("button"));
// once for the button, once for each span
expect(window.getComputedStyle).toHaveBeenCalledTimes(3);
});
});

describe("options.computedStyleSupportsPseudoElements", () => {
Expand Down
44 changes: 37 additions & 7 deletions sources/accessible-name-and-description.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
type FlatString = string & {
__flat: true;
};
type GetComputedStyle = typeof window.getComputedStyle;

/**
* interface for an options-bag where `window.getComputedStyle` can be mocked
Expand All @@ -43,7 +44,7 @@ export interface ComputeTextAlternativeOptions {
/**
* mock window.getComputedStyle. Needs `content`, `display` and `visibility`
*/
getComputedStyle?: typeof window.getComputedStyle;
getComputedStyle?: GetComputedStyle;
/**
* Set to `true` if you want to include hidden elements in the accessible name and description computation.
* Skips 2A in https://w3c.github.io/accname/#computation-steps.
Expand All @@ -69,7 +70,7 @@ function asFlatString(s: string): FlatString {
*/
function isHidden(
node: Node,
getComputedStyleImplementation: typeof window.getComputedStyle,
getComputedStyleImplementation: GetComputedStyle,
): node is Element {
if (!isElement(node)) {
return false;
Expand Down Expand Up @@ -338,6 +339,10 @@ export function computeTextAlternative(
options: ComputeTextAlternativeOptions = {},
): string {
const consultedNodes = new SetLike<Node>();
const computedStyles =
typeof Map === "undefined"
? undefined
: new Map<Element, CSSStyleDeclaration>();

const window = safeWindow(root);
const {
Expand All @@ -348,9 +353,36 @@ export function computeTextAlternative(
// window.getComputedStyle(elementFromAnotherWindow) or if I don't bind it
// the type declarations don't require a `this`
// eslint-disable-next-line no-restricted-properties
getComputedStyle = window.getComputedStyle.bind(window),
getComputedStyle: uncachedGetComputedStyle = window.getComputedStyle.bind(
window,
),
hidden = false,
} = options;
const getComputedStyle: GetComputedStyle = (
el,
pseudoElement,
): CSSStyleDeclaration => {
// We don't cache the pseudoElement styles and calls with psuedo elements
// should use the uncached version instead
if (pseudoElement !== undefined) {
throw new Error(
"use uncachedGetComputedStyle directly for pseudo elements",
);
}
// If Map is not available, it is probably faster to just use the uncached
// version since a polyfill lookup would be O(n) instead of O(1) and
// the getComputedStyle function in those environments(e.g. IE11) is fast
if (computedStyles === undefined) {
return uncachedGetComputedStyle(el);
}
const cachedStyles = computedStyles.get(el);
Copy link
Owner

Choose a reason for hiding this comment

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

Worth calling out that this lookup is not the expected O(1) but O(N) which is especially important since this may negate any benefit we get for caching.

I feel like we should only cache when the built-in Map is available. If you're testing in IE11, you don't need the caching anyway since you're in a browser already which has less issues with computing styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. i left a comment in the getComputedStyles function about the polyfill being slower. let me know if you want me to document that somewhere else as well

Copy link
Owner

Choose a reason for hiding this comment

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

I'm saying we shouldn't use the polyfill at all and only cache when we know the lookup is O(1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, i should have been clearer. i left a comment saying it was slower and that was why we were skipping using the caching if Map is 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 just removed the polyfill completely so there won't be a chance someone stumbles into the slow behavior

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 did end up having to include the global type for Map because otherwise TS complained about needing to change the target

if (cachedStyles) {
return cachedStyles;
}
const style = uncachedGetComputedStyle(el, pseudoElement);
computedStyles.set(el, style);
return style;
};

// 2F.i
function computeMiscTextAlternative(
Expand All @@ -359,7 +391,7 @@ export function computeTextAlternative(
): string {
let accumulatedText = "";
if (isElement(node) && computedStyleSupportsPseudoElements) {
const pseudoBefore = getComputedStyle(node, "::before");
const pseudoBefore = uncachedGetComputedStyle(node, "::before");
const beforeContent = getTextualContent(pseudoBefore);
accumulatedText = `${beforeContent} ${accumulatedText}`;
}
Expand All @@ -384,9 +416,8 @@ export function computeTextAlternative(
// trailing separator for wpt tests
accumulatedText += `${separator}${result}${separator}`;
});

if (isElement(node) && computedStyleSupportsPseudoElements) {
const pseudoAfter = getComputedStyle(node, "::after");
const pseudoAfter = uncachedGetComputedStyle(node, "::after");
const afterContent = getTextualContent(pseudoAfter);
accumulatedText = `${accumulatedText} ${afterContent}`;
}
Expand Down Expand Up @@ -564,7 +595,6 @@ export function computeTextAlternative(
if (consultedNodes.has(current)) {
return "";
}

// 2A
if (
!hidden &&
Expand Down
20 changes: 20 additions & 0 deletions sources/polyfills/Map.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
declare global {
class Map<K, V> {
// es2015.collection.d.ts
clear(): void;
delete(key: K): boolean;
forEach(
callbackfn: (value: V, key: K, map: Map<K, V>) => void,
thisArg?: unknown,
): void;
get(key: K): V | undefined;
has(key: K): boolean;
set(key: K, value: V): this;
readonly size: number;
}
}

// we need to export something here to make this file a module, but don't want to
// actually include a polyfill because it's potentially significantly slower than
// the native implementation
export {};
Loading