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.

Sign up for GitHub

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

Jump to bottom

add window to isInteractive #940

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

add window to isInteractive #940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4df405f
add window to isInteractive
sangikhan29 Jun 21, 2023
82d0a1a
address test failures
sangikhan29 Jun 21, 2023
129e17e
Revert "address test failures"
sangikhan29 Jun 21, 2023
bcfa012
fix test error
sangikhan29 Jun 21, 2023
63023ae
Revert "fix test error"
sangikhan29 Jun 21, 2023
cb83c99
add window to more utils
sangikhan29 Jun 21, 2023
c057166
fix includes statement
sangikhan29 Jun 27, 2023
453785f
fix getInteractives as well
sangikhan29 Jun 27, 2023
00099a7
pass control-has-associated-label
sangikhan29 Aug 15, 2023
01bd8bd
fix syntax for include
sangikhan29 Aug 15, 2023
59752a7
fix no-noninteractive-element-interactions-test
sangikhan29 Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions __mocks__/genInteractives.js
View file
Open in desktop
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions __tests__/src/rules/no-noninteractive-element-interactions-test.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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

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
View file
Open in desktop
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
View file
Open in desktop
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