inherit aria-hidden through subtree

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 2 bugs, {access})

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
related discussion was in bug 780888
Keywords: access
Assignee

Comment 2

7 years ago
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: 7 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

?
Assignee

Comment 5

7 years ago
(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 → ---
Assignee

Updated

5 years ago
Duplicate of this bug: 974238
Assignee

Updated

5 years ago
Blocks: aria-hidden
Assignee

Updated

5 years ago
No longer blocks: aria

Updated

5 years ago
Blocks: orca
Assignee

Comment 7

5 years ago
Posted 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
Assignee

Updated

5 years ago
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+
Assignee

Comment 10

4 years ago
(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?
Assignee

Comment 13

4 years ago
(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
Assignee

Comment 14

4 years ago
filed bug 1129704 on UIA thing
https://hg.mozilla.org/mozilla-central/rev/e3e113467527
Status: ASSIGNED → RESOLVED
Closed: 7 years ago4 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.
Assignee

Comment 17

4 years ago
(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.