Skip to content

Conversation

straker
Copy link
Contributor

@straker straker commented Sep 18, 2025

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 getVisibleTextRects to 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

@straker straker requested a review from a team as a code owner September 18, 2025 17:47
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

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 = 13

Note - I did not review the rest of this PR, but feel strongly enough that this needs docs to justify requesting changes.

import sanitize from '../commons/text/sanitize';
import { querySelectorAll, getScroll } from '../core/utils';

const buffer = 13;
Copy link
Member

Choose a reason for hiding this comment

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

What is this magic number?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scrollable-region-focusable should consider an element's content real content, not its content box
2 participants