Closed Bug 786143 Opened 12 years ago Closed 9 years ago

inherit aria-hidden through subtree

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file)

we expose it as object attribute, it should be propagated through subtree. We should prevent the AT crawling through accessible tree to figure out whether this accessible is inside aria-hidden subtree. For example, if focus lands into aria-hidden subtree or they get an accessible from hit test.
related discussion was in bug 780888
Keywords: access
I don't want to crawl the tree every time when object attributes are requested. I like more the approach from bug 611537 to check aria-hidden on accessible tree creation. Since aria-hidden isn't used wide then we shouldn't have performance hit to keep the tree updated.
Agreed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
(In reply to alexander :surkov from comment #2)
> I don't want to crawl the tree every time when object attributes are
> requested. I like more the approach from bug 611537 to check aria-hidden on
> accessible tree creation.

Do you mean like my old patch:
https://bug581096.bugzilla.mozilla.org/attachment.cgi?id=495085

?
(In reply to David Bolter [:davidb] from comment #3)
> Agreed.

I'm not sure I follow what you agree with and why you marked the bug as wontfix. Assuming it wasn't on purpose I reopen it.

(In reply to David Bolter [:davidb] from comment #4)
> (In reply to alexander :surkov from comment #2)
> > I don't want to crawl the tree every time when object attributes are
> > requested. I like more the approach from bug 611537 to check aria-hidden on
> > accessible tree creation.
> 
> Do you mean like my old patch:
> https://bug581096.bugzilla.mozilla.org/attachment.cgi?id=495085
> 
> ?

No, your patch was about to prune the subtree, this bug is about the inheritance of aria-hidden object attribute through the subtree.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Blocks: aria-hidden
No longer blocks: aria
Blocks: orca
Attached patch patchSplinter Review
inherit aria-hidden state in subtree, part of bug 1123360, that'll be helpful for AT to recognize events coming from aria-hidden subtree.
Assignee: nobody → surkov.alexander
Status: REOPENED → ASSIGNED
Attachment #8556523 - Flags: review?(yzenevich)
Comment on attachment 8556523 [details] [diff] [review]
patch

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

Looks good, a couple of comments below. I will build b2g right now to manually test things around and will mark the r asap.

::: accessible/generic/Accessible.cpp
@@ +1926,5 @@
>    else
>      mContextFlags &= ~eHasNameDependentParent;
> +
> +  if (mParent->IsARIAHidden() || aria::HasDefinedARIAHidden(mContent))
> +    mContextFlags |= eARIAHidden;

Lets call SetARIAHidden here.

@@ +2393,5 @@
> +  else
> +    mContextFlags &= ~eARIAHidden;
> +
> +  uint32_t length = mChildren.Length();
> +  for (uint32_t i = 0; i < length; i++)

Nit: for (...) {...}
Comment on attachment 8556523 [details] [diff] [review]
patch

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

Works as before on the device! Thanks!
Attachment #8556523 - Flags: review?(yzenevich) → review+
(In reply to Yura Zenevich [:yzen] from comment #8)
> Comment on attachment 8556523 [details] [diff] [review]
> patch
> 
> Review of attachment 8556523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, a couple of comments below. I will build b2g right now to
> manually test things around and will mark the r asap.
> 
> ::: accessible/generic/Accessible.cpp
> @@ +1926,5 @@
> >    else
> >      mContextFlags &= ~eHasNameDependentParent;
> > +
> > +  if (mParent->IsARIAHidden() || aria::HasDefinedARIAHidden(mContent))
> > +    mContextFlags |= eARIAHidden;
> 
> Lets call SetARIAHidden here.

ok, in case of adoption this makes sense I guess
(In reply to alexander :surkov from comment #10)
> (In reply to Yura Zenevich [:yzen] from comment #8)
> > Comment on attachment 8556523 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8556523 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good, a couple of comments below. I will build b2g right now to
> > manually test things around and will mark the r asap.
> > 
> > ::: accessible/generic/Accessible.cpp
> > @@ +1926,5 @@
> > >    else
> > >      mContextFlags &= ~eHasNameDependentParent;
> > > +
> > > +  if (mParent->IsARIAHidden() || aria::HasDefinedARIAHidden(mContent))
> > > +    mContextFlags |= eARIAHidden;
> > 
> > Lets call SetARIAHidden here.
> 
> ok, in case of adoption this makes sense I guess

accept adoption is a bug, so its just a waste.
Comment on attachment 8556523 [details] [diff] [review]
patch

>diff --git a/accessible/base/ARIAMap.cpp b/accessible/base/ARIAMap.cpp
>--- a/accessible/base/ARIAMap.cpp
>+++ b/accessible/base/ARIAMap.cpp
>@@ -706,17 +706,17 @@ static const AttrCharacteristics gWAIUni
>   {&nsGkAtoms::aria_controls,          ATTR_BYPASSOBJ                 | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_describedby,       ATTR_BYPASSOBJ                 | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_disabled,          ATTR_BYPASSOBJ | ATTR_VALTOKEN | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_dropeffect,                         ATTR_VALTOKEN | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_expanded,          ATTR_BYPASSOBJ | ATTR_VALTOKEN               },
>   {&nsGkAtoms::aria_flowto,            ATTR_BYPASSOBJ                 | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_grabbed,                            ATTR_VALTOKEN | ATTR_GLOBAL },
>   {&nsGkAtoms::aria_haspopup,          ATTR_BYPASSOBJ | ATTR_VALTOKEN | ATTR_GLOBAL },
>-  {&nsGkAtoms::aria_hidden,   ATTR_BYPASSOBJ_IF_FALSE | ATTR_VALTOKEN | ATTR_GLOBAL },
>+  {&nsGkAtoms::aria_hidden,            ATTR_BYPASSOBJ | ATTR_VALTOKEN | ATTR_GLOBAL }, /* handled special way */

So, now getting the UIA_PROPERTIES_ARIA_ATTR_PROPERTY_ID doesn't get aria-hidden (its unclear to me BYPASSOBJ should apply at all there)


>+aria::HasDefinedARIAHidden(nsIContent* aContent)

const nsIContent*?

>+{
>+  return aContent &&
>+    nsAccUtils::HasDefinedARIAToken(aContent, nsGkAtoms::aria_hidden) &&
>+    !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_hidden,
>+                           nsGkAtoms::_false, eCaseMatters);

So, this is basically its set and its not false, but the only case that's true and you need to care about is the attribute is set to true, so why not just check its true?
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > > Lets call SetARIAHidden here.
> > 
> > ok, in case of adoption this makes sense I guess
> 
> accept adoption is a bug, so its just a waste.

we don't allow that but it happens

(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> >+  {&nsGkAtoms::aria_hidden,            ATTR_BYPASSOBJ | ATTR_VALTOKEN | ATTR_GLOBAL }, /* handled special way */
> 
> So, now getting the UIA_PROPERTIES_ARIA_ATTR_PROPERTY_ID doesn't get
> aria-hidden (its unclear to me BYPASSOBJ should apply at all there)

ok, good point, I'll file bug for that

> >+  return aContent &&
> >+    nsAccUtils::HasDefinedARIAToken(aContent, nsGkAtoms::aria_hidden) &&
> >+    !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_hidden,
> >+                           nsGkAtoms::_false, eCaseMatters);
> 
> So, this is basically its set and its not false, but the only case that's
> true and you need to care about is the attribute is set to true, so why not
> just check its true?

I think error handling requires us to map everything not defined and not false to true
filed bug 1129704 on UIA thing
https://hg.mozilla.org/mozilla-central/rev/e3e113467527
Status: ASSIGNED → RESOLVED
Closed: 12 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > > > Lets call SetARIAHidden here.
> > > 
> > > ok, in case of adoption this makes sense I guess
> > 
> > accept adoption is a bug, so its just a waste.
> 
> we don't allow that but it happens

do you know that for a fact? any way  its stupid and counter productive to run code to paper over bugs.

> > >+  return aContent &&
> > >+    nsAccUtils::HasDefinedARIAToken(aContent, nsGkAtoms::aria_hidden) &&
> > >+    !aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_hidden,
> > >+                           nsGkAtoms::_false, eCaseMatters);
> > 
> > So, this is basically its set and its not false, but the only case that's
> > true and you need to care about is the attribute is set to true, so why not
> > just check its true?
> 
> I think error handling requires us to map everything not defined and not
> false to true

that's crazy, do you have a document supporting that view? I can't find any.
(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> (In reply to alexander :surkov from comment #13)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > 
> > > > > Lets call SetARIAHidden here.
> > > > 
> > > > ok, in case of adoption this makes sense I guess
> > > 
> > > accept adoption is a bug, so its just a waste.
> > 
> > we don't allow that but it happens
> 
> do you know that for a fact? any way  its stupid and counter productive to
> run code to paper over bugs.

I recall we run into it a while ago but in general I'm not against the idea of subtree adoption.

> > > So, this is basically its set and its not false, but the only case that's
> > > true and you need to care about is the attribute is set to true, so why not
> > > just check its true?
> > 
> > I think error handling requires us to map everything not defined and not
> > false to true
> 
> that's crazy, do you have a document supporting that view? I can't find any.

http://www.w3.org/WAI/PF/aria-implementation/#document-handling_author-errors
(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> > (In reply to alexander :surkov from comment #13)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > > 
> > > > > > Lets call SetARIAHidden here.
> > > > > 
> > > > > ok, in case of adoption this makes sense I guess
> > > > 
> > > > accept adoption is a bug, so its just a waste.
> > > 
> > > we don't allow that but it happens
> > 
> > do you know that for a fact? any way  its stupid and counter productive to
> > run code to paper over bugs.
> 
> I recall we run into it a while ago but in general I'm not against the idea
> of subtree adoption.

I haven't seen any assertions from it lately so I assume the bugs are fixed.  I think I'm against the idea unless it causes big perf problems on something.

> 
> > > > So, this is basically its set and its not false, but the only case that's
> > > > true and you need to care about is the attribute is set to true, so why not
> > > > just check its true?
> > > 
> > > I think error handling requires us to map everything not defined and not
> > > false to true
> > 
> > that's crazy, do you have a document supporting that view? I can't find any.
> 
> http://www.w3.org/WAI/PF/aria-implementation/#document-handling_author-errors

welp that's... special
> > http://www.w3.org/WAI/PF/aria-implementation/#document-handling_author-errors
> 
> welp that's... special

also that section seems to be a contradiction? you say its an author error but you also specify the behavior that should occur.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: