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

Conversation

sangikhan29
Copy link

Addresses #932

@sangikhan29 sangikhan29 changed the title add window to isInteractive [DRAFT]add window to isInteractive Jun 21, 2023
@ljharb ljharb requested a review from jessebeach June 25, 2023 01:49
@ckundo
Copy link

ckundo commented Jun 26, 2023

blocked by #941 !

@jessebeach
Copy link
Collaborator

blocked by #941 !

I saw this as well over the weekend. Trying to get this train back on the rails.

@sangikhan29 sangikhan29 marked this pull request as ready for review June 28, 2023 13:25
@ljharb ljharb changed the title [DRAFT]add window to isInteractive add window to isInteractive Aug 11, 2023
@ljharb ljharb marked this pull request as draft August 11, 2023 23:46
@ljharb
Copy link
Member

ljharb commented Aug 11, 2023

Rebased, and moved "draft" from the title to the PR status. @sangikhan29, please mark as ready for review whenever it is.

Comment on lines +52 to +59
{ 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>' },
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.

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #940 (59752a7) into main (64bfea6) will decrease coverage by 0.27%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #940      +/-   ##
==========================================
- Coverage   99.29%   99.02%   -0.27%     
==========================================
  Files         104      105       +1     
  Lines        1555     1640      +85     
  Branches      523      576      +53     
==========================================
+ Hits         1544     1624      +80     
- Misses         11       16       +5     
Files Changed Coverage Δ
src/util/isInteractiveElement.js 100.00% <100.00%> (ø)
src/util/isInteractiveRole.js 100.00% <100.00%> (ø)
src/util/isNonInteractiveElement.js 100.00% <100.00%> (ø)
src/util/isNonInteractiveRole.js 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

&& 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

@sangikhan29 sangikhan29 marked this pull request as ready for review August 15, 2023 18:37
@jessebeach
Copy link
Collaborator

I'm seeing the tasks and commentary come through email. Work-work is a bit bananas lately. As soon as we get through the end-of-the-year push and slide into the holidays, I'll have time to focus on this repo.

@ljharb
Copy link
Member

ljharb commented Nov 2, 2023

Sounds good :-) no pressure.

@ckundo
Copy link

ckundo commented Mar 16, 2024

hi friends, bumping this in case there's still an appetite for it!

@ljharb ljharb marked this pull request as draft September 4, 2024 06:00
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.

4 participants