fix(scrollable-region-focusable): do not fail elements whose contents are fully visible without scrolling #4884
Conversation
… are fully visible with out scrolling
There was a problem hiding this comment.
The added magic number should be documented, something like:
/**
* This is a magic number that is magical and important. It is
* responsible for many things in the universe.
*/
const SOME_MAGIC_NUMBER = 13Note - I did not review the rest of this PR, but feel strongly enough that this needs docs to justify requesting changes.
| clientRects.push(...getContentRects(textNode)); | ||
| }); | ||
|
|
||
| return clientRects.length ? clientRects : [boundingRect]; |
There was a problem hiding this comment.
I don't think this is quite right:
- I think there are situations where you'd want to include the whole bounding client rect even if text nodes are present (eg, visual content elements, and maybe elements that have CSS properties like
background-imagethat might give meaning to the whole client rect) - I think there are situations where either a text node rect or the whole element might be hidden for reasons other than the scroll container, where we'd probably want to ignore it for the purposes of this matches
There was a problem hiding this comment.
Agreed, only looking at text here is going to miss problems.
| rect.left - boundingRect.left + rect.width > | ||
| node.clientWidth + buffer || | ||
| rect.top - boundingRect.top + rect.height > node.clientHeight + buffer |
There was a problem hiding this comment.
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:
- check all 4 directions, or
- identify which direction(s) the user could still scroll further in, and check only those directions
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.
| hasContentVirtual(elm, true, true) | ||
| ); | ||
| function hasScrollableContent(node, virtualNode, boundingRect) { | ||
| return querySelectorAll(virtualNode, '*').some(vNode => { |
There was a problem hiding this comment.
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 * query hundreds of times on a page.
There was a problem hiding this comment.
Since it's the last part of the if statement, I think this should be fine. It'll only run if the other conditions are met first, plus this isn't doing anything it wasn't doing before.
| isComboboxPopup(virtualNode) === false && | ||
| // And there's something actually worth scrolling to | ||
| isNoneEmptyElement(virtualNode) | ||
| hasScrollableContent(node, virtualNode, boundingRect) |
There was a problem hiding this comment.
Why are you passing boundingRect in? You're also giving it virtualNode, which has that as a prop. Doesn't seem necessary.
| clientRects.push(...getContentRects(textNode)); | ||
| }); | ||
|
|
||
| return clientRects.length ? clientRects : [boundingRect]; |
There was a problem hiding this comment.
Agreed, only looking at text here is going to miss problems.
| // (elm, noRecursion, ignoreAria) | ||
| hasContentVirtual(elm, true, true) | ||
| ); | ||
| function hasScrollableContent(node, virtualNode, boundingRect) { |
There was a problem hiding this comment.
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.
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 elm._subgrid), and then start again and loop over every cell / element within the subgrid in order to find all elements to see if they have a text node or is a graphical object.
Doing this required a bit of change to current tests as they were using the exact technique that this ticket tries to solve, that is creating a larger scroll within the scrolling parent that was just empty scroll area. To fix that I either had to add more content that would scroll and/or update the size of the scrolling parent to make sure the content would scroll.
I tried to use
getVisibleTextRectsto determine the rect of the contents, but it also filters out rects either outside the bounding rect of the element or creates a smaller subrect that falls completely within the bounding rect if it's too large. Both cases were not what we needed for the check since we want to know the exact dimensions without modification. So I instead copied the main part of the function and put it in the matches directly.I also made sure that the tests took into account non-text content (such as images) and to fail / ignore the scrolling parent if the content needed to scroll.
Closes: #4535