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

Correction: revise allowed roles for iframe, object, embed #370

Closed
wants to merge 7 commits into from

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented Nov 8, 2021

closes #368

Removes role allowances for iframe, object and embed per reasoning provided in the linked issue.


Need at least two checkers to accept this change before we can merge.


Preview | Diff


Preview | Diff

closes #368

Removes role allowances for `iframe`, `object` and `embed` per reasoning provided in the linked issue.
Copy link

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

I would really prefer not to see any of these changes go in.

Comment on lines -862 to -959
<a href="#index-aria-presentation">`presentation`</a>
or <a href="#index-aria-none">`none`</a>.

Choose a reason for hiding this comment

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

I don't think this is a good change. Role=none on iframes serves a very important function for VoiceOver. It's a way to have iframe content treated as part of the same page, rather than a completely isolated context.

Copy link
Member

Choose a reason for hiding this comment

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

@WilcoFiers did you see the discussion at #368 ?

Choose a reason for hiding this comment

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

Yes I did.

Copy link
Member Author

Choose a reason for hiding this comment

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

these work with webkit, for now...

ignored with firefox (because iframes are focusable).

even if they suppress the iframe discoverability in the primary document, once navigating into the contents i'm finding that 'frame' is still announced. will post more results later, but really the only major benefit here is obscuring iframes from macOS webkit / VO users.

Choose a reason for hiding this comment

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

I seem to recall this being advertised as a best practice when ARIA first came out. There was a lot of misunderstanding of what role=presentation meant on an iframe, but ultimately, it does have an intended purpose - to communicate to the browser and AT that the content of the frame should be treated as part of the parent page and not as a sub-page. The fact that some browsers and ATs don't support this doesn't change the fact that the spec intended for the browsers to support this scenario. I think ARIA and the browser vendors need to determine who's going to budge, but as it stands now, this is perfectly valid and meaningful to put in your code.

Copy link

Choose a reason for hiding this comment

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

See #370 (comment). I'm not saying the ARIA spec should be changed. But this spec is primarily targeted at conformance checkers. It seems pretty bad to have conformance checkers telling authors something is conformant when it doesn't work across multiple browsers. Yes, we should figure out a path forward on that, but until then, isn't it better to avoid user pain by advising authors to avoid doing things which don't work across browsers?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree with @jcsteh.

i'm sorry i haven't gotten around to compiling all my test data yet. it's coming... just have to work through other things first.

Choose a reason for hiding this comment

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

That depends on the intent of your checker. If checkers only made decisions based on broad browser support, every checker would have flagged any use of ARIA as a failure when ARIA was first released since basically only one browser supported it at the time. Some checkers focus more on technical specifications whereas others look more at best practices - there's a spectrum. This issue feels like it falls somewhat inbetween, and this PR feels like it's trying to deprecate these scenarios and codify it into technical specification based on what's in the wild rather than on whether or not it makes sense to do so.

The unfortunately bit with this particular case is that this was heavily advertised in the early days as an intended pattern. Wilco's note that there exists an implementation that meets that pattern reinforces that. So, it's hard to say it can't be done when it has obviously been done.

So far, I haven't seen a discussion as to whether this pattern makes sense or not - just some notes that it's not easy to support it.

Roles:
<a href="#index-aria-application">`application`</a>,
<a href="#index-aria-document">`document`</a>,
<a href="#index-aria-img">`img`</a>,

Choose a reason for hiding this comment

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

There are legit use cases for using frames or objects to load images. For example loading in an SVG that needs to use scripts, or that loads external fonts. I don't think this should be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

again, not ready will full results. but initial findings do make me think that under certain circumstances img is ok. application and document provide no real benefit so far. and in some cases break things.

Comment on lines -859 to -956
<a href="#index-aria-application">`application`</a>,
<a href="#index-aria-document">`document`</a>,

Choose a reason for hiding this comment

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

What could possibly be wrong with these?

Copy link
Member Author

Choose a reason for hiding this comment

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

no positive benefit so far. can break access to content.

@scottaohara
Copy link
Member Author

@WilcoFiers, just wanted to respond to let you know I am not ignoring your responses. Just been very busy as of late, but i have been slowly writing test result data for these elements. quick preview is that there are definitely some cases where specifying roles may be helpful (particularly img on objects that render images), but document/application roles either aren't useful, or they can actually make the content inaccessible in some cases.

Will report more as soon as i can.

@WilcoFiers
Copy link

@scottaohara Sure thing. I'm not familiar enough with how application / document interact with iframe / object to say either way. It's the other cases that concern me. Would be interested to know though if there's anything interesting you can do with those, or what problems might occur with them.

@jcsteh
Copy link

jcsteh commented Nov 23, 2021

To be clear, I'm not necessarily saying role="none" on iframes is a bad idea. What I am saying is that the support across implementations is currently inconsistent and fixing that is non-trivial. Given that this spec primarily exists for conformance checkers, it seems important that it reflects the reality of what is actually supported. Otherwise, authors will believe they are compliant but it won't actually work across browsers in reality.

role="img" is less clear to me even from a theoretical perspective. As I noted in #368, role="img" is supposed to make children presentational. I don't see how browsers can technically support that for iframe documents given site isolation, etc.

@scottaohara
Copy link
Member Author

As I just have not had the time to finalize all the necessary work to complete and revise this PR, I'm closing this down and will instead create new PRs and necessary issues for the different elements that were to be covered by this change.

@scottaohara scottaohara removed the needs implementation commitment Cannot merge into spec until implementations in conformance checkers has been confirmed. label Mar 29, 2023
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.

Problems with allowed roles for iframe, embed and object elements
5 participants