act-rules.github.io icon indicating copy to clipboard operation
act-rules.github.io copied to clipboard

Actioning feedback from Survey on iFrames

Open HelenBurge opened this issue 2 years ago • 6 comments

Updated the content from the issue raised

Closes issue(s):

  • closes #1843

Need for Call for Review: This will require a 1 week Call for Review


Pull Request Etiquette

When creating PR:

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

After creating PR:

  • [x] Add yourself (and co-authors) as "Assignees" for PR.
  • [x] Add label to indicate if it's a Rule, Definition or Chore.
  • [x] 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.
  • [x] 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.

HelenBurge avatar Jun 02 '22 10:06 HelenBurge

Might be an edge case, but if developers would like to move the focus onto the iframe via Javascript thanks to the tabindex="-1" attribute and then, for some reason, would like to trap the user inside using a temporary keyboard trap (e.g. similarly to a modal dialog), based on this rule it should be a failure but the content inside is currently behaving as expected via keyboard.

giacomo-petri avatar Jun 13 '22 07:06 giacomo-petri

@giacomo-petri - please review the updates and see if we should add an example to help resolve the edge case?

HelenBurge avatar Jun 15 '22 14:06 HelenBurge

@HelenBurge,

Review:

line 37:

According to specs (ref: 7.1.2 Related browsing contexts),

Certain elements (for example, iframe elements) can instantiate further browsing contexts. These elements are called browsing context containers. Each browsing context container has a nested browsing context, which is either a browsing context or null. It is initially null.

if I understand correctly, in line 37, test target is referring to iframe elements. If that's true and our goal is to mark iframe elements with negative tabindex value that contain elements that are visible and part of the sequential focus navigation as failures, I would replace nested browsing context with browsing context container.

Original:

For each test target, the [nested browsing context][] does not have a negative number as a tabindex [attribute value][].

Proposal:

For each test target, the browsing context container does not have a negative number as a tabindex [attribute value][].

About my previous comment:

Scenario I've highlighted in my previous comment might be common for chat widgets, usually managed via iframe. Usually, activating the toggle chat button, an iframe is loaded and receives focus. If the iframe is managed as a dialog, with the possibility to dismiss it using a button contained in it and part of the sequential focus navigation one option could be to set tabindex="-1" to the iframe and move focus on it.

I'm not saying this is the optimal and unique solution, but it won't necessary fail the 2.1.1 criterion and in particular this rule. Content inside will still be operable using the keyboard.

Giacomo

giacomo-petri avatar Jun 16 '22 08:06 giacomo-petri

Should we mark also new Passed Example 1 (This iframe element contains a link that is not part of [sequential focus navigation][] because of its tabindex.) as inapplicable now, due to the new applicability definition "This rule applies to any iframe element that contains elements that are both [visible][] and part of the [sequential focus navigation][]."?

Edit: I've edited my previous comment removing last paragraph: "We should add instead an example with an iframe with no tabindex attribute (or tabindex="0", or both) and both visible and SFN content." as it doesn't match neither rule title nor description.

Should we instead add "with negative tabindex value" to the applicability section? This rule applies to any iframe element with negative tabindex value that contains elements that are both [visible][] and part of the [sequential focus navigation][].

Hi Giacomo, after reviewing your feedback, this is applicable, but not relevant to the work in this pull request. Could you possibly create a new pull request to action your feedback?

HelenBurge avatar Jul 11 '22 14:07 HelenBurge

I've:

  • Updated the title and description of the rule to match the new content.
  • Beefed up the background note with technical mumbo-jumbo, cleaned up the description of Failed Example.
  • Added a Passed example where there is no tabindex on the iframe (we do not want to imply that all iframe need a tabindex).

I've ultimately decided to leave the Expectation about "negative tabindex".

The local tab-order (tabindex-ordered focus navigation scope) contain both focusable areas (second point) + all focus navigation scope owners (first point). Later when flattening it, items that are not focusable areas (i.e. the scope owners) are replaced by their (tabbable content) (2nd point in the algorithm).

So, if I get that correctly, an iframe is a scope owner and therefore included in the scope of its container, but it is not necessarily a focusable area (this may be browser dependent) and therefore not in the flattened scope. So, we cannot have an expectation that the iframe is in the flattened scope.

This does also answer @giacomo-petri question from CG call yesterday: iframe are not necessarily focusable areas (this is actually UA dependent due to the the "or the element is determined by the user agent to be focusable" bit 😞), and therefore are (possibly) dropped out of the flattened scope (and the final tab-order) of the full page.


Concrete case:

<html>
<iframe id="A" srcdoc="<button>A</button>" />
<iframe id="B" tabindex="0" srcdoc="<button>B</button>" />
<iframe id="C" tabindex="-1" srcdoc="<button>C</button>" />
<button>D</button>
</html>
  1. Each of the 3 iframe has one element in its tabindex-ordered focus navigation scope: the button inside it.
  2. The main document has 3 elements in its TOFNS: the first and second iframe, and button D; the 3rd iframe is excluded from it due to its negative tabindex (first point of TOFNS).
  3. When flattening the document's TOFNS, to build the actual sequential navigation order:
    • the first iframe is not a focusable area (UA dependent), so it is replaced by its own TOFNS (point 2.2)
    • the second iframe is a focusable area (due to its own tabindex), so its TOFNS is inserted after it (point 2.3)
    • the third iframe is not even considered because it in not in the document's TOFNS.
    • the button D stays here, it's part of the clone built in step 1 and is just skipped over at step 2.1.

So, the TOFNS are:

  • iframe#A: {button A}
  • iframe#B: {button B}
  • iframe#C: {button C}
  • document: {iframe#A, iframe#B, ~iframe#C removed due to negative tabindex,~ button D}

And the flattening (assuming iframe#A is not a focusable area) yield { ~iframe#A removed as not focusable,~ button A, iframe#B, button B, button D}

Anyway, iframe#A should quite certainly pass the rule, but is not necessarily part of the final tab-order, so I kept the "no negative tabindex" expectation 😁

Jym77 avatar Jul 29 '22 09:07 Jym77

@giacomo-petri Agreed with your comments. I try to manage both sides (frontloading test target and being correct) by negating stuff (matching the expectation). Let me know what you think of it.

Jym77 avatar Aug 04 '22 07:08 Jym77