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

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.

(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.

inherit aria-hidden state in subtree, part of bug 1123360, that'll be helpful for AT to recognize events coming from aria-hidden subtree.

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!

(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

(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.