exclude aria-hidden="true" elements from the tree

NEW
Assigned to

Status

()

Core
Disability Access APIs
3 years ago
11 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
The idea was create aria-hidden="true" accessible subtrees but break parent-child relation between them and host tree, so that you never can go down to aria-hidden tree from the main tree but you can crawl up from aria-hidden="true" subtree to main subtree. Basically the idea is quite similar to shadow DOM content.

That means accessible name, group position and navigation will ignore aria-hidden subtees. Additionally we can fix getText() and getChildAtPoint implementations to exclude aria-hidden subtrees. But it allows to keep references from main document to aria-hidden subtrees, for example, maintain aria-labelledby relations.

Iirc we discussed this idea at last AT all-hands meeting and people were saying that should be good alternative to Webkit implementation. If that idea doesn't have some oversights then I suggest to give it a try on Nightly.
(Assignee)

Comment 1

3 years ago
oh, and one major difference: you will receive all events coming from aria-hidden="true" as it was a normal content, so for example if the user moves the focus into aria-hidden="true" content then the accessible focus will follow it.

Comment 2

3 years ago
For the record, this will work fine for NVDA and presumably other screen readers. I still think this could cause issues for other ATs (e.g. magnifiers), but I don't know enough to comment with any authority.
(Assignee)

Comment 3

3 years ago
(In reply to James Teh [:Jamie] from comment #2)
> For the record, this will work fine for NVDA and presumably other screen
> readers. I still think this could cause issues for other ATs (e.g.
> magnifiers)

we could not touch hit testing for now. Would it be a problem for NVDA?
(In reply to alexander :surkov from comment #0)
> The idea was create aria-hidden="true" accessible subtrees but break
> parent-child relation between them and host tree, so that you never can go
> down to aria-hidden tree from the main tree but you can crawl up from
> aria-hidden="true" subtree to main subtree. Basically the idea is quite
> similar to shadow DOM content.

I'm not exactly thrilled by what implementing that means

> That means accessible name, group position and navigation will ignore
> aria-hidden subtees. Additionally we can fix getText() and getChildAtPoint
> implementations to exclude aria-hidden subtrees. But it allows to keep
> references from main document to aria-hidden subtrees, for example, maintain
> aria-labelledby relations.

I'm not really convinced AccessibleAtPoint and maybe GetText should ignore it.

> Iirc we discussed this idea at last AT all-hands meeting and people were
> saying that should be good alternative to Webkit implementation. If that
> idea doesn't have some oversights then I suggest to give it a try on Nightly.

I don't remember saying it was a good idea, but I guess its less bad than not exposing the tree at all.

Comment 5

3 years ago
(In reply to alexander :surkov from comment #3)
> we could not touch hit testing for now. Would it be a problem for NVDA?
It would mean NVDA's mouse tracking hits aria-hidden content, which is not desirable. However, we're already dealing with that now, so that's not new. It does mean you should not get rid of the hidden:true attribute.

Comment 6

3 years ago
(In reply to alexander :surkov from comment #1)
> oh, and one major difference: you will receive all events coming from
> aria-hidden="true" as it was a normal content, so for example if the user
> moves the focus into aria-hidden="true" content then the accessible focus
> will follow it.

Out of curiosity, if we ascend the tree from the object emitting the event, what will we find?
(Assignee)

Comment 7

3 years ago
(In reply to Joanmarie Diggs from comment #6)
> (In reply to alexander :surkov from comment #1)
> > oh, and one major difference: you will receive all events coming from
> > aria-hidden="true" as it was a normal content, so for example if the user
> > moves the focus into aria-hidden="true" content then the accessible focus
> > will follow it.
> 
> Out of curiosity, if we ascend the tree from the object emitting the event,
> what will we find?

the idea is to move to the main tree

Comment 8

3 years ago
(In reply to alexander :surkov from comment #7)
> (In reply to Joanmarie Diggs from comment #6)
> > (In reply to alexander :surkov from comment #1)
> > > oh, and one major difference: you will receive all events coming from
> > > aria-hidden="true" as it was a normal content, so for example if the user
> > > moves the focus into aria-hidden="true" content then the accessible focus
> > > will follow it.
> > 
> > Out of curiosity, if we ascend the tree from the object emitting the event,
> > what will we find?
> 
> the idea is to move to the main tree

Ok, sounds sane. :) When you have something to try, I'll check it out. Thanks!
(Assignee)

Comment 9

3 years ago
here's a try [1]. Yura, could you look at jsat test failures please?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8e0cbd3b40b
Flags: needinfo?(yzenevich)
(Assignee)

Updated

3 years ago
Assignee: nobody → surkov.alexander
(Assignee)

Comment 10

3 years ago
Created attachment 8576181 [details] [diff] [review]
patch
(Assignee)

Updated

3 years ago
Attachment #8576181 - Flags: review?(yzenevich)
After further investigation with Alex it looks like AccessFu assumes some things when vc.position is attempted to be shifted to within aria-hidden subtree and we can't find previos/next siblings. I'll fix the AccessFu bit.
Flags: needinfo?(yzenevich)
Comment on attachment 8576181 [details] [diff] [review]
patch

> Accessible::RemoveChild(Accessible* aChild)
> {
>   if (!aChild)
>     return false;
> 
>-  if (aChild->mParent != this || aChild->mIndexInParent == -1)
>+  if (aChild->mParent != this)
>     return false;
> 
>+  if (aChild->mIndexInParent == -1) {
>+    if (aChild->Parent() == this) { // unbind the shadow content

redundant check

> void
>-Accessible::SetARIAHidden(bool aIsDefined)
>+Accessible::ToggleARIAHidden(bool aIsDefined)

old name makes more sense since it takes an argument instead of always flipping value.

>+
>+    Accessible* parent = mParent;
>+    parent->InvalidateChildren(); // will invalidate shadow content too
>+    parent->EnsureChildren();

that's an unnecessarily massive hammer when you can just find the spot to insert it with the TreeWalker.

>+  nsClassHashtable<nsPtrHashKey<const Accessible>, nsTArray<Accessible*> >
>+    mShadowContentMap;

So, I can't actually see any place this is used for something other than being kept up to date, so why does it need to exist?
(Assignee)

Comment 13

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

> > void
> >-Accessible::SetARIAHidden(bool aIsDefined)
> >+Accessible::ToggleARIAHidden(bool aIsDefined)
> 
> old name makes more sense since it takes an argument instead of always
> flipping value.

i don't mind

> >+
> >+    Accessible* parent = mParent;
> >+    parent->InvalidateChildren(); // will invalidate shadow content too
> >+    parent->EnsureChildren();
> 
> that's an unnecessarily massive hammer when you can just find the spot to
> insert it with the TreeWalker.

that's not how we keep tree updates, hopefully one day we can end up with something better

> >+  nsClassHashtable<nsPtrHashKey<const Accessible>, nsTArray<Accessible*> >
> >+    mShadowContentMap;
> 
> So, I can't actually see any place this is used for something other than
> being kept up to date, so why does it need to exist?

it's used to shutdown aria-hidden subtrees
> > >+
> > >+    Accessible* parent = mParent;
> > >+    parent->InvalidateChildren(); // will invalidate shadow content too
> > >+    parent->EnsureChildren();
> > 
> > that's an unnecessarily massive hammer when you can just find the spot to
> > insert it with the TreeWalker.
> 
> that's not how we keep tree updates, hopefully one day we can end up with
> something better

are you trying to say there is a case in which you think it will not work? or just that you don't want to do it that way because its not the way its done in older places?

> > >+  nsClassHashtable<nsPtrHashKey<const Accessible>, nsTArray<Accessible*> >
> > >+    mShadowContentMap;
> > 
> > So, I can't actually see any place this is used for something other than
> > being kept up to date, so why does it need to exist?
> 
> it's used to shutdown aria-hidden subtrees

where?  I don't see how its helpful in doing that.
(Assignee)

Comment 15

3 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #14)

> are you trying to say there is a case in which you think it will not work?
> or just that you don't want to do it that way because its not the way its
> done in older places?

2nd

> > > So, I can't actually see any place this is used for something other than
> > > being kept up to date, so why does it need to exist?
> > 
> > it's used to shutdown aria-hidden subtrees
> 
> where?  I don't see how its helpful in doing that.

see InvalidateShadowContent
(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> 
> > are you trying to say there is a case in which you think it will not work?
> > or just that you don't want to do it that way because its not the way its
> > done in older places?
> 
> 2nd

saying new things shouldn't be done as well as they can because its different seems like a weak argument, do you have some justification for it?

> > > > So, I can't actually see any place this is used for something other than
> > > > being kept up to date, so why does it need to exist?
> > > 
> > > it's used to shutdown aria-hidden subtrees
> > 
> > where?  I don't see how its helpful in doing that.
> 
> see InvalidateShadowContent

oh, ugh that's horrible but I can't think of anything better either :(
(Assignee)

Comment 17

3 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> (In reply to alexander :surkov from comment #15)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > 
> > > are you trying to say there is a case in which you think it will not work?
> > > or just that you don't want to do it that way because its not the way its
> > > done in older places?
> > 
> > 2nd
> 
> saying new things shouldn't be done as well as they can because its
> different seems like a weak argument, do you have some justification for it?

it's out of scope of this bug
I was also hoping for HIDE ans SHOW events to be fired when aria hidden is set to false and true on the sub-tree, in particular so I could get access to targetNextSibling and targetPreviousSibling. Would that be acceptable for orca and nvda?
Flags: needinfo?(jdiggs)
Flags: needinfo?(jamie)

Comment 19

3 years ago
When other objects (web or native) change their showing state independent of ARIA, we get events for those state changes. So as a general rule, I don't see why it's a problem. My one concern would be if the sub-tree is ginormous and thousands of elements suddenly have a state change. Event floods suck mightily. That said, hopefully authors will not have a use case for hiding/showing ginormous subtrees programmatically. So put me down as a "fine by me" -- unless Jamie or Alex have a reason for me to be concerned which I've not thought of. ;)

Thank you for asking btw!
Flags: needinfo?(jdiggs)
Comment on attachment 8576181 [details] [diff] [review]
patch

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

removing the r? for now
Attachment #8576181 - Flags: review?(yzenevich)
(Assignee)

Comment 21

3 years ago
Here's a summary.

If we do nothing. Pros: current approach, adopted by NVDA, Firefox OS screen reader, it works. Cons: quite different from Chrome/Safari impl, not adopted yet by Orca/VoiceOver and probably won't be. So we look for different approach, i.e. unattach aria-hidden content from the tree. In this case aria-hidden content doesn't exist for AT until the user goes into it. There are two possible scenarios making it happen. 1) aria-hidden="true" is set on subtree the user is in and 2) the user moves into existing aria-hidden="true" content, for example, by tabbing into it. If AT wants to ignore aria-hidden content (maybe on case by case basis) then AT needs to know next/prev accessibles to aria-hidden content. We could introduce new relations exposed on aria-hidden root to provides this info, for example, INSERTION_POINT_BEFORE and INSERTION_POINT_AFTER. Aslo, as Yura noticed, we probably have to fire show/hide events when aria-hidden content, that'd be universal way to handle it if AT is going to ignore aria-hidden subtrees in general.

Comment 22

3 years ago
(In reply to Yura Zenevich [:yzen] from comment #18)
> I was also hoping for HIDE ans SHOW events to be fired when aria hidden is
> set to false and true on the sub-tree, in particular so I could get access
> to targetNextSibling and targetPreviousSibling. Would that be acceptable for
> orca and nvda?
I can't see a problem with that.

(In reply to alexander :surkov from comment #21)
> If AT wants to
> ignore aria-hidden content (maybe on case by case basis)
Do you mean if an AT wants to ignore aria-hidden content even though the user entered it?

> then AT needs to
> know next/prev accessibles to aria-hidden content. We could introduce new
> relations exposed on aria-hidden root to provides this info
I don't quite follow. If an AT chooses to ignore the content even though the user has entered it, they'll just see that it is hidden and ignore it. We'll probably need <groan> the invisible state or something for that. Navigating next/previous from aria-hidden content will just return the next/previous node like it always did.
Flags: needinfo?(jamie)
(Assignee)

Comment 23

3 years ago
(In reply to James Teh [:Jamie] from comment #22)
> (In reply to Yura Zenevich [:yzen] from comment #18)
> > I was also hoping for HIDE ans SHOW events to be fired when aria hidden is
> > set to false and true on the sub-tree, in particular so I could get access
> > to targetNextSibling and targetPreviousSibling. Would that be acceptable for
> > orca and nvda?
> I can't see a problem with that.
> 
> (In reply to alexander :surkov from comment #21)
> > If AT wants to
> > ignore aria-hidden content (maybe on case by case basis)
> Do you mean if an AT wants to ignore aria-hidden content even though the
> user entered it?

that depends on AT, right? We should make both scenarios doable.

> > then AT needs to
> > know next/prev accessibles to aria-hidden content. We could introduce new
> > relations exposed on aria-hidden root to provides this info
> I don't quite follow. If an AT chooses to ignore the content even though the
> user has entered it, they'll just see that it is hidden and ignore it.

In this case AT need a way to get out of aria-hidden content, i.e. it needs to find aria-hidden insertion point in the tree. I suggested to have relations for that.

> We'll
> probably need <groan> the invisible state or something for that. Navigating
> next/previous from aria-hidden content will just return the next/previous
> node like it always did.

if it's unattached from tree then no, aria-hidden="true" is no in children of parent anymore.

Comment 24

3 years ago
Oh. Ug. That's ugly. When you said detached from tree, I assumed you meant you wouldn't be able to get to the hidden content by traversing children, but the hidden content would still be linked to the tree via parent, next and previous navigation. Instead, if I understand correctly, you're saying it'd be an entirely separate tree and we have to use special relations to traverse. Is there a way we can do the former? Obviously, it makes navigation asymmetric, but I think that's acceptable in this case, since you ideally wouldn't hit hidden content in the first place.
(Assignee)

Comment 25

3 years ago
(In reply to James Teh [:Jamie] from comment #24)
> Oh. Ug. That's ugly. When you said detached from tree, I assumed you meant
> you wouldn't be able to get to the hidden content by traversing children,
> but the hidden content would still be linked to the tree via parent, next
> and previous navigation.

what MSAA/IA2 methods are used for navigation to next/previous siblings?

> Instead, if I understand correctly, you're saying
> it'd be an entirely separate tree and we have to use special relations to
> traverse.

the only reason of these relations is I thought MSAA doesn't provide anything for prev/next siblings.

Comment 26

3 years ago
IAccessible::accNavigate, which Mozilla does implement. That said, MSDN does say accNavigate is deprecated, though I think that's ridiculous...
(Assignee)

Comment 27

3 years ago
(In reply to James Teh [:Jamie] from comment #26)
> IAccessible::accNavigate, which Mozilla does implement. That said, MSDN does
> say accNavigate is deprecated, though I think that's ridiculous...

Ok, so we can do that then. What about show/hide events for aria-hidden content?

Comment 28

3 years ago
See comment 22; show/hide should be fine. However, I think the invisible state should also be exposed on hidden accessibles.
(Assignee)

Comment 29

3 years ago
(In reply to James Teh [:Jamie] from comment #28)
> See comment 22; show/hide should be fine. However, I think the invisible
> state should also be exposed on hidden accessibles.

I think we can do this. Do we need to unattach it from the tree if they expose invisible state?

Comment 30

3 years ago
I still think you should detach it, yes. Aside from anything else, Orca wants this.
(Assignee)

Comment 31

3 years ago
If aria-hidden content is inserted/removed from DOM then would you expect show/hide/reorder events? If subtree of aria-hidden content is changed?

Comment 32

3 years ago
Yes, as well as textInserted/textRemoved. That is, it should be as if the content was display: none and changed to display: block.
(Assignee)

Comment 33

3 years ago
So just to make sure we're on the same page, do we fire show/hide/reorder/text change events in both cases
1) aria-hidden content is inserted/removed from the DOM or its display style is flipped
2) aria-hidden attribute is toggled on the content

Comment 34

3 years ago
For both, yes. Obviously, if content is added to the tree and it has aria-hidden="true", you don't want to fire an event for that, since (as far as a11y is concerned) it is hidden.
(Assignee)

Comment 35

3 years ago
That sounds for me as contradiction. Let me illustrate, so if I do
div.innerHTML = "<div aria-hidden=true>haha</div>; // no events
div.innerHTML = ""; // no events again

or show/reorder/textchange for 1st call and hide/reorder/textchange for 2nd?

Comment 36

3 years ago
In either case, there'll be no content inside the outer div according to the accessibility tree, so there's no need to fire events.
(Assignee)

Comment 37

3 years ago
(In reply to James Teh [:Jamie] from comment #36)
> In either case, there'll be no content inside the outer div according to the
> accessibility tree, so there's no need to fire events.

agree, but would they be harmful? they are still sort of in the tree and having no evening for their change might be problematic, no?

Comment 38

3 years ago
(In reply to alexander :surkov from comment #37)
> > In either case, there'll be no content inside the outer div according to the
> > accessibility tree, so there's no need to fire events.
> agree, but would they be harmful? they are still sort of in the tree
They could be harmful because an AT might think they're part of the real tree when they aren't. I guess an AT could check symmetric relationships, states, attributes, etc. to figure this out, but this is ugly, is a potential performance hit and raises questions about why they're being filtered out of the tree in the first place.
(Assignee)

Comment 39

3 years ago
what about children of aria-hidden content, keeping silence as a tree wasn't mutated?

Comment 40

3 years ago
If the tree is exposed due to focus being inside it, maybe. Otherwise, probably not, since the spec says the tree shouldn't be exposed otherwise.
(Assignee)

Comment 41

3 years ago
Created attachment 8623006 [details] [diff] [review]
patch

* invisible state on ARIA hidden
* show/hide events on ARIA hidden change
* failing jsat tests
Attachment #8576181 - Attachment is obsolete: true
Attachment #8623006 - Flags: review?(yzenevich)
Created attachment 8624452 [details] [diff] [review]
Bug 1123360 jsat patch

Alex, this is the patch to fix test_content_integration test.
Comment on attachment 8623006 [details] [diff] [review]
patch

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

Taking this off for now.
Attachment #8623006 - Flags: review?(yzenevich)
(Assignee)

Comment 44

3 years ago
Created attachment 8624810 [details] [diff] [review]
patch

a bit more ugliness, hope for one day rework
Attachment #8623006 - Attachment is obsolete: true
Attachment #8624810 - Flags: review?(yzenevich)
(In reply to alexander :surkov from comment #44)
> Created attachment 8624810 [details] [diff] [review]
> patch
> 
> a bit more ugliness, hope for one day rework

Hi Alex, so the original test case passes. However I get a really weird failure where I get unexpected events for live regions with static aria-hidden="true" (present in the document) when I do not actually change their state. Some more detail:
https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/jsat/test_live_regions.html#448
Is set to be aria hidden by default. However when I run the tests, I get a SHOW event for this element completely out of place. I do not change the attribute at all for the element (e.g. comment out the section that does).
(Assignee)

Comment 46

3 years ago
(In reply to Yura Zenevich [:yzen] from comment #45)

> Hi Alex, so the original test case passes. However I get a really weird
> failure where I get unexpected events for live regions with static
> aria-hidden="true" (present in the document) when I do not actually change
> their state. Some more detail:
> https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/
> jsat/test_live_regions.html#448
> Is set to be aria hidden by default. However when I run the tests, I get a
> SHOW event for this element completely out of place. I do not change the
> attribute at all for the element (e.g. comment out the section that does).

More details please? or a small test case or code snippet to demo the problem
Created attachment 8628276 [details] [diff] [review]
1123360.failing.jsat.patch

This is a patch that assembles (possibly incorrectly) all Alex's patches together with changes to accessFu.
At this point, while running test_content_integration test I get the following assertion:
https://pastebin.mozilla.org/8838243
Comment on attachment 8624810 [details] [diff] [review]
patch

removing for now.
Attachment #8624810 - Flags: review?(yzenevich)
(Assignee)

Comment 49

3 years ago
(In reply to Yura Zenevich [:yzen] On PTO until July 28th from comment #47)
> Created attachment 8628276 [details] [diff] [review]
> 1123360.failing.jsat.patch
> 
> This is a patch that assembles (possibly incorrectly) all Alex's patches
> together with changes to accessFu.
> At this point, while running test_content_integration test I get the
> following assertion:
> https://pastebin.mozilla.org/8838243

this one? is it on try server?

[Child 28772] ###!!! ASSERTION: Couldn't get the native NSView parent we need to connect the accessibility hierarchy!: '*aOutView', file /Volumes/firefoxos/B2G/gecko/accessible/mac/RootAccessibleWrap.mm, line 50
Hi Alex, here's the log for the crash:
https://etherpad.mozilla.org/3F2OMHzOZx
(Assignee)

Comment 51

3 years ago
Created attachment 8659913 [details] [diff] [review]
patch

fixed test
Attachment #8624452 - Attachment is obsolete: true
Attachment #8624810 - Attachment is obsolete: true
Attachment #8628276 - Attachment is obsolete: true
Attachment #8659913 - Flags: review?(yzenevich)
Comment on attachment 8659913 [details] [diff] [review]
patch

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

The patch itself looks good. However the aria-live tests fail because there are some unexpected show events being fired for elements that have aria-hidden set to true by default (without changes to the actual element (to false)). Any thoughts?

::: accessible/generic/DocAccessible.cpp
@@ +1303,5 @@
> +
> +  shadowRoots->AppendElement(aShadowContent);
> +  aShadowContent->BindToParent(aParent, -1);
> +
> +  if (aBuildSubtree)

nit: {}

@@ +2084,5 @@
> +
> +        nsTArray<Accessible*>* treeRoots = mShadowContentMap.Get(parent);
> +        if (treeRoots) {
> +          treeRoots->RemoveElement(aChild);
> +          if (treeRoots->IsEmpty())

nit: {}
See the question above
Flags: needinfo?(surkov.alexander)
Comment on attachment 8659913 [details] [diff] [review]
patch

>+   * Propogate or remove the context flag into/from the subtree.

nit: "propagate ..."
Hi Jamie,

I wanted to confirm something about some of the event behaviour for elements with aria-hidden set to true. If the element is added or removed from DOM for example, should we expect no show or hide event respectively for such elements? Hopefully that question makes sense
Flags: needinfo?(jamie)

Comment 56

3 years ago
Right. If aria-hidden="true", the node doesn't exist as far as accessibility consumers are concerned (unless the author does something stupid like allow it to be focused, which is author error and is thus only handled to give the user a chance to work around the brokenness). Yes, this is potentially ugly, but so is aria-hidden.
Flags: needinfo?(jamie)
Comment on attachment 8659913 [details] [diff] [review]
patch

This is probably pretty out of date, removing r? for now.
Attachment #8659913 - Flags: review?(yzenevich)
See Also: → bug 780888

Updated

11 months ago
Flags: needinfo?(surkov.alexander)
You need to log in before you can comment on or make changes to this bug.