775621 - Add pivot traversal flag for ignoring aria-hidden

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Add traversal flag for aria-hidden

I guess if we ever remove aria-hidden nodes from the tree this will become obsolete...

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 643898 [details] [diff] [review]
Add traversal flag for aria-hidden

Review of attachment 643898 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why you want to support of aria-hidden, afaik there's no agreement on it yet.

::: accessible/public/nsIAccessiblePivot.idl
@@ +194,5 @@
>    /* Pre-filters */
>    const unsigned long PREFILTER_INVISIBLE     = 0x00000001;
>    const unsigned long PREFILTER_OFFSCREEN     = 0x00000002;
>    const unsigned long PREFILTER_NOT_FOCUSABLE = 0x00000004;
> +  const unsigned long PREFILTER_HIDDEN        = 0x00000008;

ARIA_HIDDEN

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +566,5 @@
>        return NS_OK;
> +
> +    if (nsIAccessibleTraversalRule::PREFILTER_HIDDEN & mPreFilter) {
> +      nsCOMPtr<nsIPersistentProperties> attributes;
> +      aAccessible->GetAttributes(getter_AddRefs(attributes));

a11y attributes getting is expensive, you may want to check aria-hidden attribute instead, it's equivalent now

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 643898 [details] [diff] [review]
> Add traversal flag for aria-hidden
> 
> Review of attachment 643898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why you want to support of aria-hidden, afaik there's no
> agreement on it yet.
> 

It is useful, imho. The most common case are areas that are in a negative offset, or lower z-index, so are visually obscured, and then have some CSS (or JS) transition that brings them in to view. Adding a CSS "display" property may cause flicker and other unintended consequences like prematurely hiding in a hide animation. So being able to semantically mark a subtree as hidden without affecting layout allows making web aps more accessible without too much overhead.

Anyway, I didn't mean to start a discussion about the use of aria-hidden. The fact is that it is out there, and our AT should support it.

Comment 4

[alexander :surkov (:asurkov)]

I meant there's no agreement on aria-hidden implementation.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #4)
> I meant there's no agreement on aria-hidden implementation.

What specifically? The implementation looks straight forward. I just noticed that with this patch, tabzilla (the menu dropdown on mozilla websites) works correctly. So the fact is that it is out there, and being used.

Comment 6

[alexander :surkov (:asurkov)]

some says it should be mapped to visibility state what means you should do nothing to support it. we expose it as object attribute what you rely on but the spec says aria hidden is inherited, we don't do this what makes our aria-hidden implementation not really useful.

on the another hand you provide separate constant for aria-hidden what means you think that somebody might want to have a way to ignore aria-hidden content or to ignore not visible content or both. Does the user really need this feature?

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 643898 [details] [diff] [review]
Add traversal flag for aria-hidden

canceling review until we get agreement (bugzilla spams me)

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #6)
> some says it should be mapped to visibility state what means you should do
> nothing to support it. we expose it as object attribute what you rely on but
> the spec says aria hidden is inherited, we don't do this what makes our
> aria-hidden implementation not really useful.
> 

Those are questions for the API/hierarchy. As for ATs, it is pretty clear that content marked as aria-hidden, and its subtree should be ignored. That doesn't seem to be disputed.

> on the another hand you provide separate constant for aria-hidden what means
> you think that somebody might want to have a way to ignore aria-hidden
> content or to ignore not visible content or both. Does the user really need
> this feature?

That makes a lot of sense. If our core implementation changes (to marking it and all descendants as invisible), then we would remain with an obsolete flag on our hands. I think it makes sense to integrate this with PREFILTER_INVISIBLE so behaviour remains consistent.

Of course, if aria-hidden is implemented as removing the object subtree the transition won't be smooth. So another option would be always enforcing aria-hidden without any special flags. Blah...

This needs to be resolved one way or another since aria-hidden is in the wild, I am partial to adding it to PREFILTER_INVISIBLE.

Comment 9

[alexander :surkov (:asurkov)]

From AT perspective I'd go with PREFILTER_INVISIBLE.

For aria-hidden support you can do two things
1) implement it on AT side (hey, it seems you are the first AT who supports it)
2) rely on Gecko support
2.1) do what you do (you say it's a Gecko problem that aria-hidden doesn't work properly)
2.2) do something else like think of aria-hidden implementation on browser side

your choice is 2.1?

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #9)
> From AT perspective I'd go with PREFILTER_INVISIBLE.
> 
> For aria-hidden support you can do two things
> 1) implement it on AT side (hey, it seems you are the first AT who supports
> it)
> 2) rely on Gecko support
> 2.1) do what you do (you say it's a Gecko problem that aria-hidden doesn't
> work properly)
> 2.2) do something else like think of aria-hidden implementation on browser
> side
> 
> your choice is 2.1?

The pivot API is traditionally an AT thing, so I kind of see 2.1 and 1 as the same thing. I'll write up another patch soon.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

I filed spin off bug 780888.

I think the gecko a11y engine should prune the aria-hidden tree (See bug 581096 comment 21) but I didn't think we could get team agreement (talking in circles) so we ended up going softer. Given Safari and IE prune the tree, I think we need to do the same.

Eitan and I discussed this a couple of weeks ago and decided he should go with a flexible approach so that he isn't blocked on the pruning work. Let's get clear agreement on bug 780888 and get it done.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #10)

> > your choice is 2.1?
> 
> The pivot API is traditionally an AT thing, so I kind of see 2.1 and 1 as
> the same thing. I'll write up another patch soon.

Implementations are different:
1) means the pivot operates on DOM to support aria-hidden, i.e. pivot has own implementation of aria-hidden
2.1) means the pivot relies on a11y API and existing implementation

That 2.1 approach is not really useful since your AT doesn't get a support of ARIA hidden. This work is just reserve for the future what should be perfectly fine though.

Comment 13

[Eitan Isaacson [:eeejay]]

Coming back to this, after a really long time. Sorry.

(In reply to alexander :surkov from comment #2)
> Comment on attachment 643898 [details] [diff] [review]
> Add traversal flag for aria-hidden
> 
> Review of attachment 643898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why you want to support of aria-hidden, afaik there's no
> agreement on it yet.
> 
> ::: accessible/public/nsIAccessiblePivot.idl
> @@ +194,5 @@
> >    /* Pre-filters */
> >    const unsigned long PREFILTER_INVISIBLE     = 0x00000001;
> >    const unsigned long PREFILTER_OFFSCREEN     = 0x00000002;
> >    const unsigned long PREFILTER_NOT_FOCUSABLE = 0x00000004;
> > +  const unsigned long PREFILTER_HIDDEN        = 0x00000008;
> 
> ARIA_HIDDEN
> 

OK

> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +566,5 @@
> >        return NS_OK;
> > +
> > +    if (nsIAccessibleTraversalRule::PREFILTER_HIDDEN & mPreFilter) {
> > +      nsCOMPtr<nsIPersistentProperties> attributes;
> > +      aAccessible->GetAttributes(getter_AddRefs(attributes));
> 
> a11y attributes getting is expensive, you may want to check aria-hidden
> attribute instead, it's equivalent now

Done.

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #12)
> (In reply to Eitan Isaacson [:eeejay] from comment #10)
> 
> > > your choice is 2.1?
> > 
> > The pivot API is traditionally an AT thing, so I kind of see 2.1 and 1 as
> > the same thing. I'll write up another patch soon.
> 
> Implementations are different:
> 1) means the pivot operates on DOM to support aria-hidden, i.e. pivot has
> own implementation of aria-hidden
> 2.1) means the pivot relies on a11y API and existing implementation
> 
> That 2.1 approach is not really useful since your AT doesn't get a support
> of ARIA hidden. This work is just reserve for the future what should be
> perfectly fine though.

Still not clear about these options. I agree with you that aria-hidden should not prune the tree. And allowing an AT to opt-in to hiding or not hiding aria-hidden makes sense. The use case for using it is when elements are still visible on screen, but only in a way that  suggests their existence, drawers in gaia[1], is one example, and the slide show that Dominic suggested in bug #780888 is another.

So I guess this is option 2.1.

1. http://telefonicaid.github.io/Gaia-UI-Building-Blocks/index.html#widgets/drawer/

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: Add traversal flag for aria-hidden

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 753767 [details] [diff] [review]
Add traversal flag for aria-hidden

Review of attachment 753767 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +582,5 @@
> +
> +    if (nsIAccessibleTraversalRule::PREFILTER_ARIA_HIDDEN & mPreFilter) {
> +      nsAutoString hidden;
> +      aAccessible->GetContent()->GetAttr(kNameSpaceID_None, nsGkAtoms::aria_hidden, hidden);
> +      if (hidden.EqualsLiteral("true") || hidden.EqualsLiteral("1")) {

1 is not allowed value by ARIA iirc. If I understand right than any defined not false value should be treated as true but it's better to check with spec.

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Bug 775621 - Add traversal flag for aria-hidden. r=surkov

Comment 18

[Eitan Isaacson [:eeejay]]

Attachment: Introduce aria-hidden tests for pivot API

Comment 19

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5aefeafc61f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed24a16edc4

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5aefeafc61f9
https://hg.mozilla.org/mozilla-central/rev/eed24a16edc4