-
Notifications
You must be signed in to change notification settings - Fork 886
fix(target-size): ignore position: fixed elements that are offscreen when page is scrolled #5066
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
Changes from all commits
c990f12
d9533f1
2685586
a975fc1
e469362
29135e3
f87b6b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import memoize from '../../core/utils/memoize'; | ||
| import { nodeLookup } from '../../core/utils'; | ||
|
|
||
| /** | ||
| * Determines if an element is inside a position:fixed subtree, even if the element itself is positioned differently. | ||
| * @param {VirtualNode|Element} node | ||
| * @param {Boolean} [options.skipAncestors] If the ancestor tree should not be used | ||
| * @return {Boolean} The element's position state | ||
| */ | ||
| export default function isFixedPosition(node, { skipAncestors } = {}) { | ||
| const { vNode } = nodeLookup(node); | ||
|
|
||
| // detached element | ||
| if (!vNode) { | ||
| return false; | ||
| } | ||
|
|
||
| if (skipAncestors) { | ||
| return isFixedSelf(vNode); | ||
| } | ||
|
|
||
| return isFixedAncestors(vNode); | ||
| } | ||
|
|
||
| /** | ||
| * Check the element for position:fixed | ||
| */ | ||
| const isFixedSelf = memoize(function isFixedSelfMemoized(vNode) { | ||
| return vNode.getComputedStylePropertyValue('position') === 'fixed'; | ||
| }); | ||
|
|
||
| /** | ||
| * Check the element and ancestors for position:fixed | ||
| */ | ||
| const isFixedAncestors = memoize(function isFixedAncestorsMemoized(vNode) { | ||
| if (isFixedSelf(vNode)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (!vNode.parent) { | ||
| return false; | ||
| } | ||
|
|
||
| return isFixedAncestors(vNode.parent); | ||
| }); | ||
|
WilcoFiers marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import getComposedParent from './get-composed-parent'; | |
| import getElementCoordinates from './get-element-coordinates'; | ||
| import getViewportSize from './get-viewport-size'; | ||
| import { nodeLookup } from '../../core/utils'; | ||
| import isFixedPosition from './is-fixed-position'; | ||
|
|
||
| function noParentScrolled(element, offset) { | ||
| element = getComposedParent(element); | ||
|
|
@@ -37,39 +38,43 @@ function isOffscreen(element, { isAncestor } = {}) { | |
| return undefined; | ||
| } | ||
|
|
||
| let leftBoundary; | ||
| const docElement = document.documentElement; | ||
| const styl = window.getComputedStyle(domNode); | ||
| const dir = window | ||
| .getComputedStyle(document.body || docElement) | ||
| .getPropertyValue('direction'); | ||
| const coords = getElementCoordinates(domNode); | ||
| const isFixed = isFixedPosition(domNode); | ||
| const coords = isFixed | ||
| ? domNode.getBoundingClientRect() | ||
| : getElementCoordinates(domNode); | ||
|
|
||
| // Consider 0 height/ width elements at origin visible | ||
| if (coords.top === 0 && coords.bottom === 0) { | ||
| return false; | ||
| } | ||
| if (coords.left === 0 && coords.right === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| // bottom edge beyond | ||
| if ( | ||
| coords.bottom < 0 && | ||
| coords.bottom <= 0 && | ||
| (noParentScrolled(domNode, coords.bottom) || styl.position === 'absolute') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to bypass this for fixed elements?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What situation would propose that we should?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this code is buggy. It's not a new problem. I looked at fixing it while I was in here but it's too complex. I don't want to rush into it so I opened an issue for it instead #5069 |
||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (coords.left === 0 && coords.right === 0) { | ||
| //This is an edge case, an empty (zero-width) element that isn't positioned 'off screen'. | ||
| return false; | ||
| const viewportSize = getViewportSize(window); | ||
| if (isFixed && coords.top >= viewportSize.height) { | ||
| return true; // Positioned below the viewport | ||
| } | ||
|
|
||
| if (dir === 'ltr') { | ||
| if (coords.right <= 0) { | ||
| return true; | ||
| } | ||
| } else { | ||
| leftBoundary = Math.max( | ||
| docElement.scrollWidth, | ||
| getViewportSize(window).width | ||
| ); | ||
| if (coords.left >= leftBoundary) { | ||
| return true; | ||
| } | ||
| const rightEdge = Math.max(docElement.scrollWidth, viewportSize.width); | ||
| if ((isFixed || dir === 'rtl') && coords.left >= rightEdge) { | ||
| return true; // Positioned right of the viewport, preventing right scrolling | ||
| } | ||
|
|
||
| if ((isFixed || dir === 'ltr') && coords.right <= 0) { | ||
| return true; // Positioned left of the viewport, preventing left scrolling | ||
| } | ||
|
|
||
| return false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| describe('dom.isFixedPosition', () => { | ||
| const isFixedPosition = axe.commons.dom.isFixedPosition; | ||
| const { queryFixture, queryShadowFixture } = axe.testUtils; | ||
|
|
||
| it('returns true for element with "position: fixed"', () => { | ||
| const vNode = queryFixture( | ||
| '<div id="target" style="position: fixed;"></div>' | ||
| ); | ||
|
|
||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns false for element without position', () => { | ||
| const vNode = queryFixture('<div id="target"></div>'); | ||
|
|
||
| assert.isFalse(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| for (const position of ['relative', 'absolute', 'sticky']) { | ||
| it(`returns false for element with "position: ${position}"`, () => { | ||
| const vNode = queryFixture( | ||
| `<div id="target" style="position: ${position}"></div>` | ||
| ); | ||
|
|
||
| assert.isFalse(isFixedPosition(vNode)); | ||
| }); | ||
| } | ||
|
|
||
| it('returns true for ancestor with position: fixed', () => { | ||
| const vNode = queryFixture( | ||
| '<div style="position: fixed;"><div><div id="target"></div></div></div>' | ||
| ); | ||
|
|
||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns true for ancestor with "position: fixed" even when the element is positioned differently', () => { | ||
| const vNode = queryFixture( | ||
| '<div style="position: fixed;"><div><div id="target" style="position: relative"></div></div></div>' | ||
| ); | ||
|
|
||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns false on detached elements', function () { | ||
| var el = document.createElement('div'); | ||
| el.innerHTML = 'I am not visible because I am detached!'; | ||
|
|
||
| assert.isFalse(axe.commons.dom.isFixedPosition(el)); | ||
| }); | ||
|
|
||
| describe('options.skipAncestors', () => { | ||
| it('returns false for ancestor with "position: fixed"', () => { | ||
| const vNode = queryFixture( | ||
| '<div style="position: fixed;"><div><div id="target"></div></div></div>' | ||
| ); | ||
|
|
||
| assert.isFalse(isFixedPosition(vNode, { skipAncestors: true })); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Shadow DOM', () => { | ||
| it('returns false when no element in the composed tree has position: fixed', () => { | ||
| const vNode = queryShadowFixture( | ||
| '<div id="shadow"></div>', | ||
| '<span id="target"></span>' | ||
| ); | ||
| assert.isFalse(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns true for element in shadow tree with position: fixed', () => { | ||
| const vNode = queryShadowFixture( | ||
| '<div id="shadow"></div>', | ||
| '<div id="target" style="position: fixed;"></div>' | ||
| ); | ||
|
|
||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns true when ancestor outside shadow tree has position: fixed', () => { | ||
| const vNode = queryShadowFixture( | ||
| '<div style="position: fixed;"><div id="shadow"></div></div>', | ||
| '<span id="target"></span>' | ||
| ); | ||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns true when ancestor outside shadow is fixed and target in shadow has a different position', () => { | ||
| const vNode = queryShadowFixture( | ||
| '<div style="position: fixed;"><div id="shadow"></div></div>', | ||
| '<span id="target" style="position: relative"></span>' | ||
| ); | ||
| assert.isTrue(isFixedPosition(vNode)); | ||
| }); | ||
|
|
||
| it('returns false with skipAncestors when only ancestor outside shadow tree is fixed', () => { | ||
| const vNode = queryShadowFixture( | ||
| '<div style="position: fixed;"><div id="shadow"></div></div>', | ||
| '<span id="target"></span>' | ||
| ); | ||
| assert.isFalse(isFixedPosition(vNode, { skipAncestors: true })); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFixedPositionreturnsfalsewhennodeLookupcan’t find avNode. For anElementinput this can happen simply becauseaxe.setup()/flatTreeSetup()hasn’t been run yet (even though the element is connected and actuallyposition: fixed). Since this is exported asVirtualNode|Element, consider adding a DOM-only fallback (e.g., checkgetComputedStyle(node).positionand traverse composed parents) or documenting/enforcing the requirement that a virtual tree must exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary. We can assume setup was run in commons.