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: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

Merged
merged 39 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
6693b99
chore: refactor isVisible
straker Jan 14, 2022
68f30a2
comment
straker Jan 14, 2022
c3f9f39
fix firefox
straker Jan 14, 2022
c87d1e8
Update lib/commons/dom/is-visible.js
straker Jan 17, 2022
7a9dcef
update per suggestions
straker Jan 18, 2022
3d668f0
Update lib/commons/dom/is-visible.js
straker Jan 21, 2022
0e8892a
Update lib/commons/dom/is-visible.js
straker Jan 21, 2022
2134df8
Merge branch 'refs/heads/develop' into refactor-visible
straker Jul 25, 2022
cd7871b
fixes
straker Jul 28, 2022
db3006d
revert changes
straker Aug 25, 2022
d5f74c8
is-hidden-for-everyone
straker Aug 26, 2022
b9ad968
functions done
straker Aug 31, 2022
e500486
revert files
straker Aug 31, 2022
3c34939
fix lint
straker Aug 31, 2022
a830961
finish refactor
straker Sep 1, 2022
d99849b
missed
straker Sep 1, 2022
ca9b4fd
merge develop
straker Sep 1, 2022
ca5467c
fixes
straker Sep 1, 2022
33ce4d7
revert files
straker Sep 1, 2022
64fc544
Merge branch 'develop' into refactor-visible
straker Sep 6, 2022
9edbbfe
finish tests
straker Sep 6, 2022
df4ebe5
fix polyfill
straker Sep 6, 2022
d663d86
fix test
straker Sep 6, 2022
5d4aca6
Update lib/commons/dom/is-modal-open.js
straker Sep 8, 2022
c3371e8
suggestions
straker Sep 8, 2022
04e8c2a
Update lib/commons/dom/visibility-functions.js
straker Sep 8, 2022
d63581f
suggestions
straker Sep 8, 2022
fe8cdff
merge develop
straker Sep 8, 2022
432cacc
merge develop
straker Sep 8, 2022
86b9051
suggestions
straker Sep 8, 2022
f8dadac
memoize correctly
straker Sep 9, 2022
f514f5f
memoize for real this time
straker Sep 9, 2022
b825fbb
uni tests
straker Sep 19, 2022
41328e6
guard
straker Sep 19, 2022
3c5899c
reset eslintrc
straker Sep 19, 2022
c59d8b6
Merge branch 'develop' into refactor-visible
straker Sep 19, 2022
fdddacf
merge
straker Sep 19, 2022
992613d
changes
straker Sep 20, 2022
fff04f1
fix lint
straker Sep 20, 2022
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
4 changes: 2 additions & 2 deletions lib/checks/aria/aria-allowed-role-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isVisible } from '../../commons/dom';
import { isVisibleForScreenreader } from '../../commons/dom';
import { getElementUnallowedRoles } from '../../commons/aria';

/**
Expand Down Expand Up @@ -29,7 +29,7 @@ function ariaAllowedRoleEvaluate(node, options = {}, virtualNode) {
const unallowedRoles = getElementUnallowedRoles(virtualNode, allowImplicit);
if (unallowedRoles.length) {
this.data(unallowedRoles);
if (!isVisible(virtualNode, true)) {
if (!isVisibleForScreenreader(virtualNode)) {
// flag hidden elements for review
return undefined;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/aria/aria-errormessage-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import standards from '../../standards';
import { idrefs } from '../../commons/dom';
import { tokenList } from '../../core/utils';
import { isVisible } from '../../commons/dom';
import { isVisibleForScreenreader } from '../../commons/dom';
/**
* Check if `aria-errormessage` references an element that also uses a technique to announce the message (aria-live, aria-describedby, etc.).
*
Expand Down Expand Up @@ -55,7 +55,7 @@ function ariaErrormessageEvaluate(node, options, virtualNode) {
}

if (idref) {
if (!isVisible(idref, true)) {
if (!isVisibleForScreenreader(idref)) {
this.data({
messageKey: 'hidden',
values: tokenList(attr)
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/color/color-contrast-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isVisible } from '../../commons/dom';
import { isVisibleOnScreen } from '../../commons/dom';
import {
visibleVirtual,
hasUnicode,
Expand Down Expand Up @@ -29,7 +29,7 @@ export default function colorContrastEvaluate(node, options, virtualNode) {
pseudoSizeThreshold
} = options;

if (!isVisible(node, false)) {
if (!isVisibleOnScreen(node)) {
this.data({ messageKey: 'hidden' });
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/generic/has-descendant-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { querySelectorAllFilter } from '../../core/utils';
import { isVisible, isModalOpen } from '../../commons/dom';
import { isVisibleForScreenreader, isModalOpen } from '../../commons/dom';

function hasDescendant(node, options, virtualNode) {
if (!options || !options.selector || typeof options.selector !== 'string') {
Expand All @@ -15,7 +15,7 @@ function hasDescendant(node, options, virtualNode) {
const matchingElms = querySelectorAllFilter(
virtualNode,
options.selector,
vNode => isVisible(vNode.actualNode, true)
vNode => isVisibleForScreenreader(vNode)
);
this.relatedNodes(matchingElms.map(vNode => vNode.actualNode));
return matchingElms.length > 0;
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/generic/page-no-duplicate-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import cache from '../../core/base/cache';
import { querySelectorAllFilter } from '../../core/utils';
import { isVisible, findUpVirtual } from '../../commons/dom';
import { isVisibleForScreenreader, findUpVirtual } from '../../commons/dom';

function pageNoDuplicateEvaluate(node, options, virtualNode) {
if (!options || !options.selector || typeof options.selector !== 'string') {
Expand All @@ -18,7 +18,7 @@ function pageNoDuplicateEvaluate(node, options, virtualNode) {
cache.set(key, true);

let elms = querySelectorAllFilter(axe._tree[0], options.selector, elm =>
isVisible(elm.actualNode, true)
isVisibleForScreenreader(elm)
);

// Filter elements that, within certain contexts, don't map their role.
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/keyboard/accesskeys-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { isVisible } from '../../commons/dom';
import { isVisibleOnScreen } from '../../commons/dom';

function accesskeysEvaluate(node) {
if (isVisible(node, false)) {
if (isVisibleOnScreen(node)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
this.data(node.getAttribute('accesskey'));
this.relatedNodes([node]);
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/label/explicit-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRootNode, isVisible } from '../../commons/dom';
import { getRootNode, isVisibleOnScreen } from '../../commons/dom';
import { accessibleText } from '../../commons/text';
import { escapeSelector } from '../../core/utils';

Expand All @@ -16,7 +16,7 @@ function explicitEvaluate(node, options, virtualNode) {
try {
return labels.some(label => {
// defer to hidden-explicit-label check for better messaging
if (!isVisible(label)) {
if (!isVisibleOnScreen(label)) {
return true;
} else {
return !!accessibleText(label);
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/label/hidden-explicit-label-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRootNode, isVisible } from '../../commons/dom';
import { getRootNode, isVisibleForScreenreader } from '../../commons/dom';
import { accessibleTextVirtual } from '../../commons/text';
import { escapeSelector } from '../../core/utils';

Expand All @@ -12,7 +12,7 @@ function hiddenExplicitLabelEvaluate(node, options, virtualNode) {
const id = escapeSelector(node.getAttribute('id'));
const label = root.querySelector(`label[for="${id}"]`);

if (label && !isVisible(label, true)) {
if (label && !isVisibleForScreenreader(label)) {
let name;
try {
name = accessibleTextVirtual(virtualNode).trim();
Expand Down
6 changes: 3 additions & 3 deletions lib/checks/label/multiple-label-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRootNode, isVisible, idrefs } from '../../commons/dom';
import { getRootNode, isVisibleOnScreen, isVisibleForScreenreader, idrefs } from '../../commons/dom';
import { escapeSelector } from '../../core/utils';

function multipleLabelEvaluate(node) {
Expand All @@ -10,7 +10,7 @@ function multipleLabelEvaluate(node) {

if (labels.length) {
// filter out CSS hidden labels because they're fine
labels = labels.filter(label => isVisible(label));
labels = labels.filter(label => isVisibleOnScreen(label));
straker marked this conversation as resolved.
Show resolved Hide resolved
}

while (parent) {
Expand All @@ -27,7 +27,7 @@ function multipleLabelEvaluate(node) {

// more than 1 CSS visible label
if (labels.length > 1) {
const ATVisibleLabels = labels.filter(label => isVisible(label, true));
const ATVisibleLabels = labels.filter(label => isVisibleForScreenreader(label));
// more than 1 AT visible label will fail IOS/Safari/VO even with aria-labelledby
if (ATVisibleLabels.length > 1) {
return undefined;
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/lists/only-dlitems-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isVisible } from '../../commons/dom';
import { isVisibleForScreenreader } from '../../commons/dom';
import { getRole, getExplicitRole } from '../../commons/aria';

function onlyDlitemsEvaluate(node, options, virtualNode) {
Expand All @@ -23,7 +23,7 @@ function onlyDlitemsEvaluate(node, options, virtualNode) {
const { actualNode } = childNode;
const tagName = actualNode.nodeName.toUpperCase();

if (actualNode.nodeType === 1 && isVisible(actualNode, true, false)) {
if (actualNode.nodeType === 1 && isVisibleForScreenreader(actualNode)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
const explicitRole = getExplicitRole(actualNode);

if ((tagName !== 'DT' && tagName !== 'DD') || explicitRole) {
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/lists/only-listitems-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isVisible } from '../../commons/dom';
import { isVisibleForScreenreader } from '../../commons/dom';
import { getRole } from '../../commons/aria';

function onlyListitemsEvaluate(node, options, virtualNode) {
Expand All @@ -17,7 +17,7 @@ function onlyListitemsEvaluate(node, options, virtualNode) {
return;
}

if (actualNode.nodeType !== 1 || !isVisible(actualNode, true, false)) {
if (actualNode.nodeType !== 1 || !isVisibleForScreenreader(actualNode)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand Down
4 changes: 2 additions & 2 deletions lib/checks/navigation/heading-order-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import cache from '../../core/base/cache';
import { querySelectorAllFilter, getAncestry } from '../../core/utils';
import { isVisible } from '../../commons/dom';
import { isVisibleForScreenreader } from '../../commons/dom';
import { getRole } from '../../commons/aria';

function getLevel(vNode) {
Expand Down Expand Up @@ -55,7 +55,7 @@ function headingOrderEvaluate() {
const selector = 'h1, h2, h3, h4, h5, h6, [role=heading], iframe, frame';
// TODO: es-modules_tree
const vNodes = querySelectorAllFilter(axe._tree[0], selector, vNode =>
straker marked this conversation as resolved.
Show resolved Hide resolved
isVisible(vNode.actualNode, true)
isVisibleForScreenreader(vNode.actualNode)
);

headingOrder = vNodes.map(vNode => {
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/navigation/region-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function findRegionlessElms(virtualNode, options) {
['iframe', 'frame'].includes(virtualNode.props.nodeName) ||
(dom.isSkipLink(virtualNode.actualNode) &&
dom.getElementByReference(virtualNode.actualNode, 'href')) ||
!dom.isVisible(node, true)
!dom.isVisibleForScreenreader(node)
) {
// Mark each parent node as having region descendant
let vNode = virtualNode;
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/navigation/skip-link-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { getElementByReference, isVisible } from '../../commons/dom';
import { getElementByReference, isVisibleForScreenreader } from '../../commons/dom';

function skipLinkEvaluate(node) {
const target = getElementByReference(node, 'href');
if (target) {
return isVisible(target, true) || undefined;
return isVisibleForScreenreader(target) || undefined;
}
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/shared/is-on-screen-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { isVisible, isOffscreen } from '../../commons/dom';
import { isVisibleOnScreen, isOffscreen } from '../../commons/dom';

function isOnScreenEvaluate(node) {
// From a visual perspective
return isVisible(node, false) && !isOffscreen(node);
return isVisibleOnScreen(node) && !isOffscreen(node);
straker marked this conversation as resolved.
Show resolved Hide resolved
}

export default isOnScreenEvaluate;
4 changes: 2 additions & 2 deletions lib/commons/dom/focus-disabled.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import AbstractVirtualNode from '../../core/base/virtual-node/abstract-virtual-node';
import { getNodeFromTree } from '../../core/utils';
import isHiddenWithCSS from './is-hidden-with-css';
import isHiddenForEveryone from './is-hidden-for-everyone';
// Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled
const allowedDisabledNodeNames = [
'button',
Expand Down Expand Up @@ -74,7 +74,7 @@ function focusDisabled(el) {
if (!vNode.actualNode) {
return false;
}
return isHiddenWithCSS(vNode.actualNode);
return isHiddenForEveryone(vNode);
}

return false;
Expand Down
4 changes: 2 additions & 2 deletions lib/commons/dom/get-rect-stack.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import isVisible from './is-visible';
import isVisibleOnScreen from './is-visible-on-screen';
import VirtualNode from '../../core/base/virtual-node/virtual-node';
import { getNodeFromTree, getScroll, isShadowRoot } from '../../core/utils';

Expand Down Expand Up @@ -80,7 +80,7 @@ export function createGrid(
// (we don't do this before so we can calculate stacking context
// of parents with 0 width/height)
const rect = vNode.boundingClientRect;
if (rect.width !== 0 && rect.height !== 0 && isVisible(node)) {
if (rect.width !== 0 && rect.height !== 0 && isVisibleOnScreen(node)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
addNodeToGrid(grid, vNode);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/commons/dom/has-lang-text.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { hasChildTextNodes } from './has-content-virtual';
import isVisualContent from './is-visual-content';
import isVisible from './is-visible';
import isVisibleForScreenreader from './is-visible-for-screenreader';

/**
* Check that a node has text, or an accessible name which language is defined by the
Expand All @@ -19,6 +19,6 @@ export default function hasLangText(virtualNode) {
return virtualNode.children.some(child => (
!child.attr('lang') && // non-empty lang
hasLangText(child) && // has text
isVisible(child, true) // Not hidden for AT
isVisibleForScreenreader(child) // Not hidden for AT
));
}
3 changes: 3 additions & 0 deletions lib/commons/dom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export { default as insertedIntoFocusOrder } from './inserted-into-focus-order';
export { default as isCurrentPageLink } from './is-current-page-link';
export { default as isFocusable } from './is-focusable';
export { default as isHiddenWithCSS } from './is-hidden-with-css';
export { default as isHiddenForEveryone } from './is-hidden-for-everyone';
export { default as isHTML5 } from './is-html5';
export { default as isInTabOrder } from './is-in-tab-order';
export { default as isInTextBlock } from './is-in-text-block';
Expand All @@ -33,6 +34,8 @@ export { default as isNode } from './is-node';
export { default as isOffscreen } from './is-offscreen';
export { default as isOpaque } from './is-opaque';
export { default as isSkipLink } from './is-skip-link';
export { default as isVisibleForScreenreader } from './is-visible-for-screenreader';
export { default as isVisibleOnScreen } from './is-visible-on-screen';
export { default as isVisible } from './is-visible';
export { default as isVisualContent } from './is-visual-content';
export { default as reduceToElementsBelowFloating } from './reduce-to-elements-below-floating';
Expand Down
67 changes: 67 additions & 0 deletions lib/commons/dom/is-hidden-for-everyone.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import AbstractVirtualNode from '../../core/base/virtual-node/abstract-virtual-node';
import { getNodeFromTree } from '../../core/utils';
import memoize from '../../core/utils/memoize';
import {
nativelyHidden,
displayHidden,
visibilityHidden
} from './visibility-functions';

/**
* Determine if an element is hidden from screenreaders and visual users
* @method isHiddenForEveryone
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember why we called this isHiddenWithCss. It's because for example an off screen element with aria-hidden is hidden for everyone, but this method is going to return false on it.

I'm not sure what to suggest for this, other than that maybe we need another name for this still. This one can be potentially just as misleading as what we have today.

* @memberof axe.commons.dom
* @param {VirtualNode} vNode The Virtual Node
* @param {Boolean} [ancestors=true] If the ancestor tree should be used
* @param {Boolean} recursed
* @return {Boolean} The element's visibility state
*/
export default function isHiddenForEveryone(vNode, ancestors = true, recursed) {
straker marked this conversation as resolved.
Show resolved Hide resolved
vNode = vNode instanceof AbstractVirtualNode ? vNode : getNodeFromTree(vNode);

return ancestors
? isHiddenAncestors(vNode, recursed)
: isHiddenSelf(vNode, recursed);
straker marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Check the element for visibility state
*/
const isHiddenSelf = memoize(function isHiddenSelfMemoized(vNode, recursed) {
if (nativelyHidden(vNode)) {
return true;
}

if (vNode.actualNode) {
straker marked this conversation as resolved.
Show resolved Hide resolved
const hiddenMethods = [displayHidden, visibilityHidden];

if (hiddenMethods.some(method => method(vNode, recursed))) {
return true;
}

// detached node
if (!vNode.actualNode.isConnected) {
return true;
}
}

return false;
});

/**
* Check the element and ancestors for visibility state
*/
const isHiddenAncestors = memoize(function isHiddenAncestorsMemoized(
vNode,
recursed
) {
if (isHiddenSelf(vNode, recursed)) {
return true;
}

if (vNode.parent) {
return isHiddenAncestors(vNode.parent, true);
}

return false;
});
1 change: 1 addition & 0 deletions lib/commons/dom/is-hidden-with-css.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { getNodeFromTree } from '../../core/utils';
* @param {HTMLElement} el The HTML Element
* @param {Boolean} descendentVisibilityValue (Optional) immediate descendant visibility value used for recursive computation
* @return {Boolean} the element's hidden status
* @deprecated use isHiddenForEveryone
straker marked this conversation as resolved.
Show resolved Hide resolved
*/
function isHiddenWithCSS(el, descendentVisibilityValue) {
const vNode = getNodeFromTree(el);
Expand Down
4 changes: 2 additions & 2 deletions lib/commons/dom/is-modal-open.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import isVisible from './is-visible';
import isVisibleOnScreen from './is-visible-on-screen';
import getViewportSize from './get-viewport-size';
import cache from '../../core/base/cache';
import { querySelectorAllFilter } from '../../core/utils';
Expand Down Expand Up @@ -42,7 +42,7 @@ function isModalOpen(options) {
// TODO: es-module-_tree
axe._tree[0],
'dialog, [role=dialog], [aria-modal=true]',
vNode => isVisible(vNode.actualNode)
vNode => isVisibleOnScreen(vNode)
straker marked this conversation as resolved.
Show resolved Hide resolved
);

if (definiteModals.length) {
Expand Down
8 changes: 7 additions & 1 deletion lib/commons/dom/is-offscreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ function noParentScrolled(element, offset) {
* @param {Element} element
* @return {Boolean}
*/
function isOffscreen(element) {
function isOffscreen(element, recursed) {
straker marked this conversation as resolved.
Show resolved Hide resolved
if (recursed) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this be false? I don't quite get the logic of this.

Copy link
Contributor Author

@straker straker Sep 8, 2022

Choose a reason for hiding this comment

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

It failed a test without it due to this logic and comment https://github.com/dequelabs/axe-core/blob/develop/lib/commons/dom/is-visible.js#L211-L216

// visibility is only accurate on the first element and
// position does not matter if it was already calculated

}

element = element.actualNode ? element.actualNode : element
straker marked this conversation as resolved.
Show resolved Hide resolved

let leftBoundary;
const docElement = document.documentElement;
const styl = window.getComputedStyle(element);
Expand Down
Loading