-
Notifications
You must be signed in to change notification settings - Fork 841
fix(scrollable-region-focusable): do not fail elements whose contents are fully visible without scrolling #4884
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
base: develop
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
@@ -1,21 +1,58 @@ | ||
import hasContentVirtual from '../commons/dom/has-content-virtual'; | ||
import isComboboxPopup from '../commons/aria/is-combobox-popup'; | ||
import sanitize from '../commons/text/sanitize'; | ||
import { querySelectorAll, getScroll } from '../core/utils'; | ||
|
||
// magic number of allowed negligence if element goes outside scrolling area | ||
// set to a ~1em past the scroll area. can modify if needed | ||
const buffer = 13; | ||
|
||
export default function scrollableRegionFocusableMatches(node, virtualNode) { | ||
const boundingRect = virtualNode.boundingClientRect; | ||
return ( | ||
// The element scrolls | ||
getScroll(node, 13) !== undefined && | ||
getScroll(node, buffer) !== undefined && | ||
// It's not a combobox popup, which commonly has keyboard focus added | ||
isComboboxPopup(virtualNode) === false && | ||
// And there's something actually worth scrolling to | ||
isNoneEmptyElement(virtualNode) | ||
hasScrollableContent(node, virtualNode, boundingRect) | ||
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. Why are you passing boundingRect in? You're also giving it virtualNode, which has that as a prop. Doesn't seem necessary. |
||
); | ||
} | ||
|
||
function isNoneEmptyElement(vNode) { | ||
return querySelectorAll(vNode, '*').some(elm => | ||
// (elm, noRecursion, ignoreAria) | ||
hasContentVirtual(elm, true, true) | ||
); | ||
function hasScrollableContent(node, virtualNode, boundingRect) { | ||
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. I wonder if we can use the grid to do this. That seems like it'd be more efficient. Each scrollable element should have a subgrid, right? Why not look at the subgrid, find each element that actually extends beyond the visible boundary, and then check if A. that element has text that extends the boundary, or B. that element is a graphical object with a non-empty accessible text (i.e. it's not decorative). 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. I'm not sure this would work easily. The grid was designed to do bottom-up lookups from a cell of a grid really easily, but it doesn't do top-down from a container very well. To do this we'd have to loop over every element within the grid, which requires looping over every cell of the grid, and then check if that element is itself a scroll container (checking |
||
return querySelectorAll(virtualNode, '*').some(vNode => { | ||
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. Is this going to create a performance problem? I think its worth considering if there are other ways to do this. This may end up running a 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. Since it's the last part of the |
||
const hasContent = hasContentVirtual(vNode, true, true); | ||
if (!hasContent) { | ||
return false; | ||
} | ||
|
||
return getChildTextRects(vNode).some( | ||
rect => | ||
// part or all of the element is outside the scroll area | ||
rect.left - boundingRect.left + rect.width > | ||
node.clientWidth + buffer || | ||
rect.top - boundingRect.top + rect.height > node.clientHeight + buffer | ||
Comment on lines
+32
to
+34
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. This is only checking the right and bottom sides, which might be ok if we knew the scroll container was in its leftmost/topmost scroll position, but we aren't checking that. I think this should either:
I could live with either. The latter might be better at ignoring the case where someone uses an element-hiding technique based on a negative x position, but it'd be more complicated and I could imagine it missing cases with nested different-direction scroll containers that we'd prefer it to match. |
||
); | ||
}); | ||
} | ||
|
||
function getChildTextRects(vNode) { | ||
const boundingRect = vNode.boundingClientRect; | ||
const clientRects = []; | ||
|
||
vNode.actualNode.childNodes.forEach(textNode => { | ||
if (textNode.nodeType !== 3 || sanitize(textNode.nodeValue) === '') { | ||
return; | ||
} | ||
|
||
clientRects.push(...getContentRects(textNode)); | ||
}); | ||
|
||
return clientRects.length ? clientRects : [boundingRect]; | ||
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. I don't think this is quite right:
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. Agreed, only looking at text here is going to miss problems. |
||
} | ||
|
||
function getContentRects(node) { | ||
const range = document.createRange(); | ||
range.selectNodeContents(node); | ||
return Array.from(range.getClientRects()); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.