Skip to content

Create focus-returns-trigger-9au0ou.md #2022

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

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open

Conversation

HelenBurge
Copy link
Collaborator

@HelenBurge HelenBurge commented Jan 31, 2023

First draft of the 2.4.3 focus returns to trigger rule.

Adding a rule for testing the focus returning to the trigger tests.

Closes issue(s):

Need for Call for Review:
This will require a 2 weeks Call for Review


Pull Request Etiquette

When creating PR:

  • Make sure you're requesting to pull a branch (right side) to the develop branch (left side).
  • Make sure you do not remove the "How to Review and Approve" section in your pull request description

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.
  • Link the PR to any issue it solves. This will be done automatically by referencing the issue at the top of this comment in the indicated place.
  • Optionally request feedback from anyone in particular by assigning them as "Reviewers".

When merging a PR:

  • Close any issue that the PR resolves. This will happen automatically upon merging if the PR was correctly linked to the issue, e.g. by referencing the issue at the top of this comment.

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Call for Review period. In case of disagreement, the longer period wins.

First draft of the 2.4.3 focus returns to trigger rule/
@carlosapaduarte carlosapaduarte added Rule Use this label for a new rule that does not exist already manual rule labels Jan 31, 2023
@HelenBurge HelenBurge marked this pull request as draft January 31, 2023 13:39
@dan-tripp-siteimprove
Copy link
Collaborator

Okay, I pushed a "Passed Example 1". I'd like to know what you think of it, before I create other examples. The problem is that it will be hard for you to test it. You can't just copy and paste that HTML. I think that your options are:

  1. You can copy the multiple .css and .js files to your local machine and then edit some paths. (If you want to pursue this option, let me know.)
  2. Wait until it's built and released on the ACT rules site (i.e. https://www.w3.org/WAI/standards-guidelines/act/rules/) (this option seems like the worst one to me.)
  3. Discover some other method of viewing the built HTML before it gets published on the ACT rules site. (I don't know if this option exists. Maybe Jean-Yves or Wilco would.)

@dan-tripp-siteimprove
Copy link
Collaborator

I put "Passed Example 1" on a live test page. What do you think?

@HelenBurge
Copy link
Collaborator Author

Love it Dan! Thanks for all of your hard work :)

@dan-tripp-siteimprove
Copy link
Collaborator

dan-tripp-siteimprove commented Mar 8, 2023

I've been thinking about the examples that use a link (instead of a button) to open the modal. (i.e. passed examples 4, 5 and 6, and failed examples 3 and 4.) Is it best to include these? I gather that using a link in this way is a bad practice - discussed here, for example: https://stackoverflow.com/questions/38594369/is-it-more-accessible-to-use-a-button-or-a-to-open-close-a-modal . Even if people do it, we wouldn't want to encourage it, right?

Added updates from @Jym77 feedback
@HelenBurge HelenBurge requested a review from Jym77 May 4, 2023 11:22
@HelenBurge
Copy link
Collaborator Author

I've been thinking about the examples that use a link (instead of a button) to open the modal. (i.e. passed examples 4, 5 and 6, and failed examples 3 and 4.) Is it best to include these? I gather that using a link in this way is a bad practice - discussed here, for example: https://stackoverflow.com/questions/38594369/is-it-more-accessible-to-use-a-button-or-a-to-open-close-a-modal . Even if people do it, we wouldn't want to encourage it, right?

I agree, but best to allow it as we do not say how to code, but how to test and it is possible to have links that activate modals :(

Copy link
Collaborator

@dan-tripp-siteimprove dan-tripp-siteimprove left a comment

Choose a reason for hiding this comment

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

I like these changes so far, but I think that before they're merged into the main ACT repo we need to resolve all of the 'code needed' and other 'to do' items. Because they're not fit for public consumption. At least, I think that's the protocol for these PRs to ACT - no PR until it's fit for public consumption. I'll let more senior members than myself comment on that.

@dan-tripp-siteimprove
Copy link
Collaborator

I've been thinking about the examples that use a link (instead of a button) to open the modal. (i.e. passed examples 4, 5 and 6, and failed examples 3 and 4.) Is it best to include these? I gather that using a link in this way is a bad practice - discussed here, for example: https://stackoverflow.com/questions/38594369/is-it-more-accessible-to-use-a-button-or-a-to-open-close-a-modal . Even if people do it, we wouldn't want to encourage it, right?

I agree, but best to allow it as we do not say how to code, but how to test and it is possible to have links that activate modals :(

Fair. I'll try to do that later in May.

@Jym77 Jym77 marked this pull request as ready for review February 7, 2025 13:52
@giacomo-petri
Copy link
Collaborator

Hi @HelenBurge,

I've reviewed this rule and believe it's a bit too strict. We may need to tweak it slightly:

  • Applicability:

    • the phrase "role of modal dialog" isn’t quite accurate. We typically assign the role="dialog" with a modal experience (aria-modal="true"), but perhaps a better definition would be when focus is trapped within a region activated by a trigger control that is dismissible
    • the rule seems to apply more to the element that triggers a temporarily trapped keyboard focus in a modal experience, rather than the element that "is not visible"
  • Expectation: the current expectation may be too strict. Some components, once activated, modify background content in ways that prevent focus from returning to the original trigger element. Examples include:

    • an alert dialog that reloads the page
    • a confirmation popup that dynamically updates content via ajax
    • ...

    We might need to define two separate expectations or clarify certain assumptions to account for these scenarios.

  • Accessibility Support: while this behavior has recently improved, some assistive technologies on known browsers still exhibit inconsistencies. Specifically, if an element is removed from the accessibility tree after being activated (e.g., a close button), and focus is moved via JavaScript to an element that isn’t inherently operable (e.g., a button works, but a div with tabindex="-1" may not), the AT might automatically shift focus to the closest visible element, regardless of the intended JavaScript behavior.

Copy link

netlify bot commented May 8, 2025

Deploy Preview for act-rules ready!

Name Link
🔨 Latest commit 94a8083
🔍 Latest deploy log https://app.netlify.com/projects/act-rules/deploys/685142dca195580008ac3112
😎 Deploy Preview https://deploy-preview-2022--act-rules.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@HelenBurge HelenBurge requested a review from kengdoj May 8, 2025 11:49
@HelenBurge
Copy link
Collaborator Author

Hi @HelenBurge,

I've reviewed this rule and believe it's a bit too strict. We may need to tweak it slightly:

  • Applicability:

    • the phrase "role of modal dialog" isn’t quite accurate. We typically assign the role="dialog" with a modal experience (aria-modal="true"), but perhaps a better definition would be when focus is trapped within a region activated by a trigger control that is dismissible
    • the rule seems to apply more to the element that triggers a temporarily trapped keyboard focus in a modal experience, rather than the element that "is not visible"
  • Expectation: the current expectation may be too strict. Some components, once activated, modify background content in ways that prevent focus from returning to the original trigger element. Examples include:

    • an alert dialog that reloads the page
    • a confirmation popup that dynamically updates content via ajax
    • ...

    We might need to define two separate expectations or clarify certain assumptions to account for these scenarios.

  • Accessibility Support: while this behavior has recently improved, some assistive technologies on known browsers still exhibit inconsistencies. Specifically, if an element is removed from the accessibility tree after being activated (e.g., a close button), and focus is moved via JavaScript to an element that isn’t inherently operable (e.g., a button works, but a div with tabindex="-1" may not), the AT might automatically shift focus to the closest visible element, regardless of the intended JavaScript behavior.

Hi @giacomo-petri I have agreed and updated the main file with your feedback.

@HelenBurge HelenBurge requested a review from sage-putnam May 29, 2025 14:23
@HelenBurge HelenBurge removed the request for review from Jym77 June 12, 2025 14:28
@HelenBurge
Copy link
Collaborator Author

@dan-tripp-siteimprove - please add some more examples for the rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted manual rule Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants