Skip to content
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

add window to isInteractive #940

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
6 changes: 3 additions & 3 deletions __mocks__/genInteractives.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const interactiveElementsMap = {
button: [],
canvas: [],
datalist: [],
dialog: [],
embed: [],
input: [],
'input[type="button"]': [{ prop: 'type', value: 'button' }],
Expand Down Expand Up @@ -71,7 +72,6 @@ const nonInteractiveElementsMap: {[string]: Array<{[string]: string}>} = {
del: [],
details: [],
dfn: [],
dialog: [],
dir: [],
dl: [],
dt: [],
Expand Down Expand Up @@ -140,13 +140,13 @@ const interactiveRoles = []
)
.filter((role) => (
!roles.get(role).abstract
&& roles.get(role).superClass.some((klasses) => includes(klasses, 'widget'))
&& roles.get(role).superClass.some((klasses) => includes(klasses, 'widget') || includes(klasses, 'window'))
));

const nonInteractiveRoles = roleNames
.filter((role) => (
!roles.get(role).abstract
&& !roles.get(role).superClass.some((klasses) => includes(klasses, 'widget'))
&& !roles.get(role).superClass.some((klasses) => includes(klasses, 'widget') || includes(klasses, 'window'))

// 'toolbar' does not descend from widget, but it does support
// aria-activedescendant, thus in practice we treat it as a widget.
Expand Down
6 changes: 3 additions & 3 deletions __tests__/src/rules/control-has-associated-label-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,14 @@ const alwaysValid = [
{ code: '<menuitem>Save</menuitem>' },
{ code: '<option>Save</option>' },
{ code: '<th>Save</th>' },
{ code: '<dialog>Save</dialog>' },
// Interactive Roles
{ code: '<div role="alertdialog">Save</div>' },
{ code: '<div role="button">Save</div>' },
{ code: '<div role="checkbox">Save</div>' },
{ code: '<div role="columnheader">Save</div>' },
{ code: '<div role="combobox">Save</div>' },
{ code: '<div role="dialog">Save</div>' },
Comment on lines +52 to +59
Copy link
Author

Choose a reason for hiding this comment

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

I think it's fair to expect dialog to have a label according to this.

{ code: '<div role="gridcell">Save</div>' },
{ code: '<div role="link">Save</div>' },
{ code: '<div role="menuitem">Save</div>' },
Expand Down Expand Up @@ -119,7 +122,6 @@ const alwaysValid = [
{ code: '<dd />' },
{ code: '<details />' },
{ code: '<dfn />' },
{ code: '<dialog />' },
{ code: '<dir />' },
{ code: '<dl />' },
{ code: '<dt />' },
Expand Down Expand Up @@ -162,15 +164,13 @@ const alwaysValid = [
{ code: '<ul />' },
// Non-interactive Roles
{ code: '<div role="alert" />' },
{ code: '<div role="alertdialog" />' },
{ code: '<div role="application" />' },
{ code: '<div role="article" />' },
{ code: '<div role="banner" />' },
{ code: '<div role="cell" />' },
{ code: '<div role="complementary" />' },
{ code: '<div role="contentinfo" />' },
{ code: '<div role="definition" />' },
{ code: '<div role="dialog" />' },
{ code: '<div role="directory" />' },
{ code: '<div role="document" />' },
{ code: '<div role="feed" />' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,12 @@ const alwaysValid = [
{ code: '<wbr onClick={() => {}} />;' },
{ code: '<xmp onClick={() => {}} />;' },
/* HTML elements attributed with an interactive role */
{ code: '<div role="alertdialog" onClick={() => {}} />;' },
{ code: '<div role="button" onClick={() => {}} />;' },
{ code: '<div role="checkbox" onClick={() => {}} />;' },
{ code: '<div role="columnheader" onClick={() => {}} />;' },
{ code: '<div role="combobox" onClick={() => {}} />;' },
{ code: '<div role="dialog" onClick={() => {}} />;' },
{ code: '<div role="grid" onClick={() => {}} />;' },
{ code: '<div role="gridcell" onClick={() => {}} />;' },
{ code: '<div role="link" onClick={() => {}} />;' },
Expand Down Expand Up @@ -342,15 +344,13 @@ const neverValid = [
{ code: '<div contentEditable role="article" onKeyDown={() => {}} />;', errors: [expectedError] },
/* HTML elements attributed with a non-interactive role */
{ code: '<div role="alert" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="alertdialog" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="application" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="article" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="banner" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="cell" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="complementary" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="contentinfo" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="definition" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="dialog" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="directory" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="document" onClick={() => {}} />;', errors: [expectedError] },
{ code: '<div role="feed" onClick={() => {}} />;', errors: [expectedError] },
Expand Down
4 changes: 2 additions & 2 deletions src/util/isInteractiveElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const nonInteractiveRoles = new Set(roleKeys
// 'toolbar' does not descend from widget, but it does support
// aria-activedescendant, thus in practice we treat it as a widget.
&& name !== 'toolbar'
&& !role.superClass.some((classes) => includes(classes, 'widget'))
&& !role.superClass.some((classes) => includes(classes, 'widget') || includes(classes, 'window'))
);
}).concat(
// The `progressbar` is descended from `widget`, but in practice, its
Expand All @@ -43,7 +43,7 @@ const interactiveRoles = new Set(roleKeys
// The `progressbar` is descended from `widget`, but in practice, its
// value is always `readonly`, so we treat it as a non-interactive role.
&& name !== 'progressbar'
&& role.superClass.some((classes) => includes(classes, 'widget'))
&& role.superClass.some((classes) => includes(classes, 'widget') || includes(classes, 'window'))
);
}).concat(
// 'toolbar' does not descend from widget, but it does support
Expand Down
2 changes: 1 addition & 1 deletion src/util/isInteractiveRole.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import flatMap from 'array.prototype.flatmap';
const roles = [...rolesMap.keys()];
const interactiveRoles = roles.filter((name) => (
!rolesMap.get(name).abstract
&& rolesMap.get(name).superClass.some((klasses) => includes(klasses, 'widget'))
&& rolesMap.get(name).superClass.some((klasses) => includes(klasses, 'widget') || includes(klasses, 'window'))
Copy link
Author

@sangikhan29 sangikhan29 Aug 15, 2023

Choose a reason for hiding this comment

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

I find this change to be the easiest way to implement this suggestion. But I realized that this change will also impact interactive-supports-focus, where the linter will throw an error if a dialog isn't focusable. But according to mdn, dialog is not required to be focusable although it should have a child that's focusable.

My case for requesting dialog to be interactive (reference) was for more so that it should be allowed (rather than required) to be interactive. I'd feel more comfortable just treating dialog somewhat similarly to presentation on case-by-base basis rather than have it follow all the expectations attached to an interactive role.

@jessebeach would do you have any concerns with just adding custom checks for dialog(+alertdialog) in no-static-element-interactions and no-noninteractive-element-interactions rather than setting window to be interactive? cc: @ckundo

));

// 'toolbar' does not descend from widget, but it does support
Expand Down
4 changes: 2 additions & 2 deletions src/util/isNonInteractiveElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const nonInteractiveRoles = new Set(roleKeys
// treats them both as CellRole and since gridcell is interactive, we consider
// cell interactive as well.
&& name !== 'cell'
&& !role.superClass.some((classes) => includes(classes, 'widget'))
&& !role.superClass.some((classes) => includes(classes, 'widget') || includes(classes, 'window'))
);
}).concat(
// The `progressbar` is descended from `widget`, but in practice, its
Expand All @@ -54,7 +54,7 @@ const interactiveRoles = new Set(roleKeys
// This role is meant to have no semantic value.
// @see https://www.w3.org/TR/wai-aria-1.2/#generic
&& name !== 'generic'
&& role.superClass.some((classes) => includes(classes, 'widget'))
&& role.superClass.some((classes) => includes(classes, 'widget') || includes(classes, 'window'))
);
}).concat(
// 'toolbar' does not descend from widget, but it does support
Expand Down
2 changes: 1 addition & 1 deletion src/util/isNonInteractiveRole.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import flatMap from 'array.prototype.flatmap';
const roles = [...rolesMap.keys()];
const nonInteractiveRoles = roles.filter((name) => (
!rolesMap.get(name).abstract
&& !rolesMap.get(name).superClass.some((klasses) => includes(klasses, 'widget'))
&& !rolesMap.get(name).superClass.some((klasses) => includes(klasses, 'widget') || includes(klasses, 'window'))
));

/**
Expand Down
Loading