Skip to content

chore(target-size): report non-tabbable widgets for review#3699

Merged
WilcoFiers merged 8 commits intodevelopfrom
target-size-not-tabbable
Oct 12, 2022
Merged

chore(target-size): report non-tabbable widgets for review#3699
WilcoFiers merged 8 commits intodevelopfrom
target-size-not-tabbable

Conversation

@WilcoFiers
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers commented Oct 5, 2022

This PR makes it so that focusable elements that are not in the tab order (i.e. tabindex="-1") are reported for review when they are too small / too close to other widgets, and that widgets that are too close to such elements are flag for review.

Clsoes #3695

@WilcoFiers WilcoFiers marked this pull request as ready for review October 5, 2022 17:32
@WilcoFiers WilcoFiers requested a review from a team as a code owner October 5, 2022 17:32
"fail": "Target has insufficient offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",
"incomplete": {
"default": "Potential target has insufficient offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",
"nonTabbableNeighbor": "Target has insufficient offset from a potential other target (${data.closestOffset}px should be at least ${data.minOffset}px)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is kind of vague and doesn't help the user understand that it's the tabindex that's causing the problem. We use closest neighbor in the other messages so wondering if we should use the same term here.

Suggested change
"nonTabbableNeighbor": "Target has insufficient offset from a potential other target (${data.closestOffset}px should be at least ${data.minOffset}px)"
"nonTabbableNeighbor": "Target potentially has insufficient offset from a neighbor with tabindex=-1 (${data.closestOffset}px should be at least ${data.minOffset}px)"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm sort of anticipating that inert will be a thing soon too.

Copy link
Copy Markdown
Contributor

@straker straker Oct 11, 2022

Choose a reason for hiding this comment

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

Looks like you don't have to wait long, it's now available in all browsers except Firefox, but is available in a nightly build. https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert#browser_compatibility. In that case, we should say something about how the element is not focusable instead of tabindex=-1.

Target potentially has insufficient offset from a non-focusable neighbor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On second thought, inert would make an element inapplicable in this rule because it's not focusable. I made some changes. See what you think.

"incomplete": {
"default": "Possible target has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)",
"partiallyObscured": "Possible target has insufficient size because it is partially obscured (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)",
"partiallyObscuredNonTabbable": "Target has insufficient size because it is partially obscured by a potential second target (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should mention the tabindex=-1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm sort of anticipating that inert will be a thing soon too.

"partiallyObscured": "Target has insufficient size because it is partially obscured (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)"
},
"incomplete": {
"default": "Possible target has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does "possible target" mean? The target of the rule isn't a "possible target" as it matched the rule.

]);
});

it('returns undefined if if the target is not tabbable', function () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('returns undefined if if the target is not tabbable', function () {
it('returns undefined if the target is not tabbable', function () {

]);
});

it('returns undefined if if the obscuring node is not tabbable', function () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('returns undefined if if the obscuring node is not tabbable', function () {
it('returns undefined if the obscuring node is not tabbable', function () {

Base automatically changed from target-size-nesting to develop October 11, 2022 08:54
@WilcoFiers WilcoFiers merged commit 74dd278 into develop Oct 12, 2022
@WilcoFiers WilcoFiers deleted the target-size-not-tabbable branch October 12, 2022 19:05
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.

2 participants