Closed Bug 1123360 Opened 9 years ago Closed 6 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

patch
32.89 KB, patch
Details | Diff | Splinter Review 
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.
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.
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.
(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.
(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.
(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?
(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
(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!
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: nobody → surkov.alexander
Attached patch patch (obsolete) — — Splinter Review 

    
  

        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?

    
    

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

    
    

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

    
    

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

    
    

    
  
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)

    
    

    
  
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.

    
    

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

    
    

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

    
    

    
  
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.

    
    

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

    
    

    
  
IAccessible::accNavigate, which Mozilla does implement. That said, MSDN does say accNavigate is deprecated, though I think that's ridiculous...

    
    

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

    
    

    
  
See comment 22; show/hide should be fine. However, I think the invisible state should also be exposed on hidden accessibles.

    
    

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

    
    

    
  
I still think you should detach it, yes. Aside from anything else, Orca wants this.

    
    

    
  
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?

    
    

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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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.

    
    

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

    
    

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

    
    

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

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
* 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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Bug 1123360 jsat patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


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

    
    

    
  
(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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1123360.failing.jsat.patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
(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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
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)
Flags: needinfo?(surkov.alexander)

    
    

    
  
Abandoned in favour of bug 1349223 (the full "cut the tree" approach).
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: