Closed Bug 581096 Opened 14 years ago Closed 13 years ago

aria-hidden should expose hidden attribute and fire attributechange events

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b11
Tracking Flags:
Tracking Status
blocking2.0 --- .x+
status2.0 --- wanted

People

(Reporter: MarcoZ, Assigned: davidb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [aria spec conformance])

Attachments

(2 files, 6 obsolete files)

patch 1.2 (only obj attr)
11.07 KB, patch
surkov
: review+
davidb
: approval2.0+
Details | Diff | Splinter Review
patch to land
13.30 KB, patch
Details | Diff | Splinter Review 
Brought to my attention by Rich Schwerdtfeger. See spec here:
http://www.w3.org/WAI/PF/aria-implementation/#mapping_general

Right now, elements with aria-hidden="true" get the invisible state set.
OK, I should probably take this one.
Assignee: nobody → bolterbugz
Attached patch WIP (obsolete) — — Splinter Review 

        Attachment #459811 -
        Flags: feedback?(marco.zehe)

    
    

    
  
Comment on attachment 459811 [details] [diff] [review]
WIP

>+                           nsAccessibilityAtoms::aria_selected,

You mean the new nsAccessibilityAtoms::aria_hidden atom here. ;-)

The approach is sound, but this still has so many unanswered questions that we should discuss further and refine. Also I'd obviously like a test for this once we agree on what exactly should be happening if aria-hidden hides a tree but that tree contains focusable elements, for example.

f=me for this first WIP, but not ready to take this yet.

        Attachment #459811 -
        Flags: feedback?(marco.zehe) → feedback+

    
    

    
  
Agreed on all counts. (aria_hidden fixed locally)

Perhaps I should add a drill down check for focusable elements?

    
    

    
  
Comment on attachment 459811 [details] [diff] [review]
WIP

Marking this early WIP obsolete in case it scares anyone :)

Note: we'll need to watch aria-hidden changes like we watch changes to visibility currently. I didn't run our test suite yet but I expect failures. New tests will need to be written of course, as per Marco's comment.

        Attachment #459811 -
        Attachment is obsolete: true

    
    

    
  
Hmmm, changed my mind on the approach for this... my current thinking is to just add the STATE_INVISIBLE and/or STATE_UNAVAILABLE bit(s) to the root node's state but to otherwise keep the tree.

Jamie, would that be enough for NVDA to ignore the subtree?

    
    

    
  
Or maybe better yet, AT can just look at the object attribute?

    
    

    
  
OK email discussion happening among some usual suspects. Sounds like object attribute isn't enough (we currently expose this of course).

    
    

    
  
NVDA's Gecko virtual buffer code currently only ignores nodes with a width and height of 0. I'd prefer to use the invisible state and we used to do this, but we had problems with content being ignored when it shouldn't be; i.e. Gecko exposing the invisible state when we didn't expect it. I have a vague recollection that this happened with floated content, but I can't quite remember how to reproduce it. I don't think we were ever able to come up with a test case that isolated the behaviour.

I think the invisible state is the best approach, but at present, it won't work in practical terms due to the above issue. However, maybe we can somehow get that issue resolved without breaking backwards compatibility and then use the invisible state for this as well.

    
    

    
  
(In reply to comment #9)
> I think the invisible state is the best approach

While the spec says we shouldn't expose aria-hidden elements in tree then we can do nothing here.

It sounds that's the case when ARIA spec is changed without our participation. Previously ARIA impl guide was more polite to implementations of aria-hidden. David, what's objection of this change?

    
    

    
  
(In reply to comment #9)
> I have a vague
> recollection that this happened with floated content, but I can't quite
> remember how to reproduce it. I don't think we were ever able to come up with a
> test case that isolated the behaviour.

That is a pickle. Do you think it would be too dangerous to go back to using invisible state? We have seen churn in our event system that might have corrected this problem. Of course, if you can recreate the issue please report a bug -- even if vague.

(In reply to comment #10)
> (In reply to comment #9)
> > I think the invisible state is the best approach
> 
> While the spec says we shouldn't expose aria-hidden elements in tree then we
> can do nothing here.

The ARIA implementation guide is the key to success here (interoperable a11y). We have a lot of pull there.

> It sounds that's the case when ARIA spec is changed without our participation.
> Previously ARIA impl guide was more polite to implementations of aria-hidden.

Yes, my leave (family issue) in April/May sure didn't help things here.

> David, what's objection of this change?

I'm not sure what you are asking?

    
    

    
  
(In reply to comment #11)

> > David, what's objection of this change?
> 
> I'm not sure what you are asking?

I mean why did they change implementation requirements for aria-hidden? Previously it was:

"This is not used in mapping to platform accessibility APIs. Instead,
use information from the browser core to determine if the element is
hidden or not. Advisory: it is incorrect use of WAI-ARIA if an element
with aria-hidden  (state)="true" is visible. The aria-hidden  property
is exposed only so that DOM-based assistive technologies can be
informed of visibility changes. However, the browser core will be able
to provide the most complete set of all truly hidden nodes."

My opinion this is more reasonable since ideally AT users should see the same thing that sighted users see. That makes me skeptic about if we need an ability to hide things visible for sighted users and not visible for blind users or vice versa.

So my question is what's the background for new behavior of aria-hidden?

    
    

    
  
(In reply to comment #9)
> NVDA's Gecko virtual buffer code currently only ignores nodes with a width and
> height of 0. I'd prefer to use the invisible state and we used to do this, but
> we had problems with content being ignored when it shouldn't be; i.e. Gecko
> exposing the invisible state when we didn't expect it. I have a vague
> recollection that this happened with floated content, but I can't quite
> remember how to reproduce it.
Mick, do you perhaps remember what the story was here better than I do?

    
    

    
  
I spun off bug 591363 for the visibility state problem.

    
    

    
  
Update: I have re-emailed the usual suspects to see if we can make a spec change here.

    
    

    
  
The current prose in the implementation guide:

"This is used in platform accessibility APIs to indicate that the element and its children are hidden and should not be exposed to assistive technology. For assistive technologies that operate directly on the user agent's DOM it indicates that the element and its children are hidden and should not be processed by the assistive technology. If the aria-hidden state is undefined, implementations MAY use information from the browser rendering engine to determine if the element is visibly hidden. Advisory: it is incorrect use of WAI-ARIA if an element with aria-hidden (state)="true" is visible. The aria-hidden property is exposed only so that DOM-based assistive technologies can be informed of visibility changes. However, the browser engine will be able to provide the most complete set of all truly hidden nodes."

          status2.0:
          --- → wanted
Summary: aria-hidden set to true should generally cause the element not to be in the accessibility tree → aria-hidden set to true should prune the tree
Whiteboard: [aria spec conformance]

    
    

    
  
OK we need to do this, and should try to get it safely into FF4.
blocking2.0: --- → final+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP (tests to come) (obsolete)
        — 
          — Splinter Review
      

    


  
This works in manual testing. Will create mochitests.

    
    

    
  
Comment on attachment 494437 [details] [diff] [review]
WIP (tests to come)


> 
>+  // We don't create accessible subtrees for elements that have the
>+  // aria-hidden=true property. This is scary but we do it for specification
>+  // conformance and interoperability; trusting the web developer.
>+  if (content->AttrValueIs(kNameSpaceID_None,
>+                           nsAccessibilityAtoms::aria_hidden,
>+                           nsAccessibilityAtoms::_true, eCaseMatters)){
>+    *aIsSubtreeHidden = true;
>+
>+    return nsnull;

what about focusable children?

>+  if (aAttribute == nsAccessibilityAtoms::aria_hidden) {
>+    if (aContent->AttrValueIs(kNameSpaceID_None,
>+                             nsAccessibilityAtoms::aria_hidden,
>+                             nsAccessibilityAtoms::_true, eCaseMatters)){
>+      FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_HIDE, aContent);
>+    }
>+    else {
>+      FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_SHOW, aContent);
>+    }

wrong, show/hide events don't update the tree

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP2 (with test) (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #494437 -
        Attachment is obsolete: true

    
    

    
  
(In reply to comment #19)
> Comment on attachment 494437 [details] [diff] [review]
> WIP (tests to come)
> 
> 
> > 
> >+  // We don't create accessible subtrees for elements that have the
> >+  // aria-hidden=true property. This is scary but we do it for specification
> >+  // conformance and interoperability; trusting the web developer.
> >+  if (content->AttrValueIs(kNameSpaceID_None,
> >+                           nsAccessibilityAtoms::aria_hidden,
> >+                           nsAccessibilityAtoms::_true, eCaseMatters)){
> >+    *aIsSubtreeHidden = true;
> >+
> >+    return nsnull;
> 
> what about focusable children?

One of the expected common use cases for aria-hidden is when something like a dialog is presented to the user and the background content has a layer placed over it which often blurs the content (maybe via an svg filter). The layer also prevents the user from using the mouse on the content, and keyboard control is restricted in some other way by the web developer.

So, if we were to look through the subtree whenever we saw an aria-hidden=true and expose nodes we "think" are focusable, we would be circumvented one of the reasons for aria-hidden.

My thinking is that we should implement it to spec (not looking for focusables), or not at all.

    
    

    
  
(In reply to comment #21)

> My thinking is that we should implement it to spec (not looking for
> focusables), or not at all.

Do I understand right if I have
<input aria-hidden="true"/> then tabbing into it and typing shouldn't announced by screen reader?

and as consequence if for example I have
<input>
<input aria-hidden="true">

and I tab from first input to second and start typing then nothing announced, however the accessible focus is on first input. Isn't it a strange?

> So, if we were to look through the subtree whenever we saw an aria-hidden=true
> and expose nodes we "think" are focusable, we would be circumvented one of the
> reasons for aria-hidden.

Isn't it the same case for role="presentation"? Do we circumvent a reason of role="presentation" when we ignore it on focusable nodes?

    
    

    
  
(In reply to comment #22)
> (In reply to comment #21)
> 
> > My thinking is that we should implement it to spec (not looking for
> > focusables), or not at all.
> 
> Do I understand right if I have
> <input aria-hidden="true"/> then tabbing into it and typing shouldn't announced
> by screen reader?
> 

Right, unfortunately this would indicate an author error.

> and as consequence if for example I have
> <input>
> <input aria-hidden="true">
> 
> and I tab from first input to second and start typing then nothing announced,
> however the accessible focus is on first input. Isn't it a strange?
> 
> > So, if we were to look through the subtree whenever we saw an aria-hidden=true
> > and expose nodes we "think" are focusable, we would be circumvented one of the
> > reasons for aria-hidden.
> 
> Isn't it the same case for role="presentation"? Do we circumvent a reason of
> role="presentation" when we ignore it on focusable nodes?

We set the precedent there. It is a different use case.

    
    

    
  
(In reply to comment #23)

> Right, unfortunately this would indicate an author error.

Ok, where's AT focus?

(In reply to comment #23)

> > Isn't it the same case for role="presentation"? Do we circumvent a reason of
> > role="presentation" when we ignore it on focusable nodes?
> 
> We set the precedent there. It is a different use case.

Any usecase of focusable role="presentation"? It sounds ARIA treats having a presentation role on focusable element as author mistake and we try to fix it. That doesn't happen with aria-hidden. So I don't get a difference.

    
    

    
  
(In reply to comment #24)
> (In reply to comment #23)

I expressed these concerns during the W3C discussions, and now I must try to remember the answers :)

> > Right, unfortunately this would indicate an author error.
> 
> Ok, where's AT focus?

I'll talk to Apple.

> (In reply to comment #23)
> 
> > > Isn't it the same case for role="presentation"? Do we circumvent a reason of
> > > role="presentation" when we ignore it on focusable nodes?
> > 
> > We set the precedent there. It is a different use case.
> 
> Any usecase of focusable role="presentation"? It sounds ARIA treats having a
> presentation role on focusable element as author mistake and we try to fix it.
> That doesn't happen with aria-hidden. So I don't get a difference.

It is just as you might guess. We pioneered role="presentation" and we didn't pioneer aria-hidden.

For "fixing" I see a few options:

1. like we do presentation, create accessibles for what we think is focusable.
2. ignore aria-hidden if there is anything in the subtree we think is focusable.
3. don't implement aria-hidden at all.
4. only create accessibles if and when focus goes into the aria-hidden subtree.

1. breaks the overlay use case.
2. breaks the overlay use case.
3. non conformant.
4. this might work.
blocking2.0: final+ → .x

    
    

    
  
I want to soften this. Let's just expose offscreen and fire show/hides, but leave the tree intact.

    
    

    
  
(In reply to comment #27)
> I want to soften this. Let's just expose offscreen and fire show/hides, but
> leave the tree intact.
Err, shouldn't we be exposing invisible, not offscreen?

    
    

    
  
(In reply to comment #28)
> (In reply to comment #27)
> > I want to soften this. Let's just expose offscreen and fire show/hides, but
> > leave the tree intact.
> Err, shouldn't we be exposing invisible, not offscreen?

Which would work better for you?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Plan B WIP (obsolete)
        — 
          — Splinter Review
      

    


  
Fires show/hide and object attribute changed. Needs reorder event?
Summary: aria-hidden set to true should prune the tree → aria-hidden should fire attributechange and show/hide events

    
    

    
  
Good news. It sounds like I am finally getting W3C agreement on not having to prune the sub-tree which is ideal.

Marco, as per our chat the try build should appear here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/dbolter@mozilla.com-9d819dee1ac7

Manual testcase coming (gotta go AFK).

    
    

    
  
(In reply to comment #29)
> > Err, shouldn't we be exposing invisible, not offscreen?
> Which would work better for you?
I believe invisible is more correct. Offscreen suggests that the item is just scrolled off the screen but could reappear if the user scrolled it back into view.

Note that this won't work for NVDA yet. We still don't handle the invisible state because of confusion as to how to do it correctly without losing desired objects/including undesired objects in our buffer as per bug 591363.

    
    

    
  
(In reply to comment #32)
> (In reply to comment #29)
> > > Err, shouldn't we be exposing invisible, not offscreen?
> > Which would work better for you?
> I believe invisible is more correct. Offscreen suggests that the item is just
> scrolled off the screen but could reappear if the user scrolled it back into
> view.
> 
> Note that this won't work for NVDA yet. We still don't handle the invisible
> state because of confusion as to how to do it correctly without losing desired
> objects/including undesired objects in our buffer as per bug 591363.

Yep but note we will fire an attribute changed event so in the meantime you can special case for aria-hidden. (I suppose in your case you might want us to report 0 dimensions for nodes with aria-hidden?)

    
    

    
  
(In reply to comment #33)

> Yep but note we will fire an attribute changed event so in the meantime you can
> special case for aria-hidden. (I suppose in your case you might want us to
> report 0 dimensions for nodes with aria-hidden?)

I think he wants us to fix bug 591363 :) If we're not in time to fix it in Fx4 timeframe than let's schedule it on next plane which should be short after fx4.

    
    

    
  
(In reply to comment #33)
> > Note that this won't work for NVDA yet. We still don't handle the invisible
> > state because of confusion as to how to do it correctly without losing desired
> > objects/including undesired objects in our buffer as per bug 591363.
> Yep but note we will fire an attribute changed event so in the meantime you can
> special case for aria-hidden. (I suppose in your case you might want us to
> report 0 dimensions for nodes with aria-hidden?)
That depends whether you want a clean solution or a hacky solution. :) Width and height of 0 is not enough; the child count must be 0 as well. I don't think an object attribute is the right approach here, since invisibility really is what we're talking about. That said, fixing bug 591363 is probably going to be a nightmare, so I'm not sure of the best approach.

    
    

    
  
Jamie, how about the IA2_STATE_DEFUNCT state? At first glance it might appear hacky, but I'm not so sure. Would that work for you?

    
    

    
  
(In reply to comment #36)
> Jamie, how about the IA2_STATE_DEFUNCT state? At first glance it might appear
> hacky, but I'm not so sure. Would that work for you?
Gecko already uses this to communicate dead objects. If the object is apparently dead, it shouldn't be linked to the tree at all.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #495085 -
        Attachment is obsolete: true

        Attachment #502077 -
        Attachment is obsolete: true

        Attachment #504491 -
        Flags: review?(surkov.alexander)

    
    

    
  
Could you please give me a summary what and why should we do making me skip bug discussions?

    
    

    
  
Comment on attachment 504491 [details] [diff] [review]
patch 1

Sure.

>+  // eARIAHidden
>+  nsStateMapEntry(&nsAccessibilityAtoms::aria_hidden, kBoolType, 0,
>+                  nsIAccessibleStates::STATE_INVISIBLE, 0),
>+

This hooks our aria state to gecko state.


> eStateMapEntryID nsARIAMap::gWAIUnivStateMap[] = {
>   eARIABusy,
>   eARIADisabled,
>   eARIAExpanded,  // Currently under spec review but precedent exists
>   eARIAHasPopup,  // Note this is technically a "property"
>+  eARIAHidden,

Make it explicit that aria-hidden is a global aria state.

>  * ARIA attribute map for attribute characteristics
>@@ -711,16 +716,17 @@ nsAttributeCharacteristics nsARIAMap::gW


>+  {&nsAccessibilityAtoms::aria_hidden,                             ATTR_VALTOKEN },/* always expose obj attr */

We don't set the bypass attributes flag, so that we will explicitly include hidden as an object attribute. This leaves the door open to AT workarounds if they don't trust our visibility state yet.


>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -1236,17 +1236,17 @@ nsAccessibilityService::HasUniversalAria

>-         // purposely ignore aria-hidden; since we use gecko for detecting this anyways
>+         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_hidden) ||

We can't ignore aria-hidden any more per spec.

>+++ b/accessible/src/base/nsDocAccessible.cpp
>+  if (aAttribute == nsAccessibilityAtoms::aria_hidden) {
>+    FireDelayedAccessibleEvent(nsIAccessibleEvent::EVENT_OBJECT_ATTRIBUTE_CHANGED,
>+                               aContent);
>+    nsRefPtr<AccEvent> event;
>+    nsAccessible* accessible = GetAccService()->GetAccessible(aContent);
>+    if (!accessible)
>+      return;
>+
>+    if (aContent->AttrValueIs(kNameSpaceID_None,
>+                              nsAccessibilityAtoms::aria_hidden,
>+                              nsAccessibilityAtoms::_true, eCaseMatters))
>+      event = new AccHideEvent(accessible, aContent, eAutoDetect);
>+    else
>+      event = new AccShowEvent(accessible, aContent, eAutoDetect);
>+
>+    if (event)
>+      FireDelayedAccessibleEvent(event);
>+
>+    return;
>+  }
>+

We should fire the object attribute changed for AT watching attributes. And we should fire show/hide as per UA Aria Implementation guide.

We can IRC details, but your hard blocker comes first of course.

    
    

    
  
(In reply to comment #40)

> We should fire the object attribute changed for AT watching attributes. And we
> should fire show/hide as per UA Aria Implementation guide.

This part bothers me. What API do you keep in mind taking about show/hide events? Since Gecko's show/hide events are result of tree changes then should we change the tree? Should we fire reorder event? Should be they coalesced with other show/hide events?

    
    

    
  
(In reply to comment #41)
> (In reply to comment #40)
> 
> > We should fire the object attribute changed for AT watching attributes. And we
> > should fire show/hide as per UA Aria Implementation guide.
> 
> This part bothers me. What API do you keep in mind taking about show/hide
> events?

MSAA: EVENT_OBJECT_SHOW, EVENT_OBJECT_HIDE
ATK: children_changed::add / remove

> Since Gecko's show/hide events are result of tree changes then should
> we change the tree?

No.

> Should we fire reorder event? Should be they coalesced with
> other show/hide events?

There was no reply on that (comment 30). It isn't clear to me. Jamie can you comment on which events you'd like to see? (You can leave details about what won't work today -- we need to just get it right)

    
    

    
  
Jamie, when in a web page, hitting NVDA+numpad8 a bunch of times takes us to the application accessible. Why does a subsequent NVDA+numpad2 not succeed?

    
    

    
  
(In reply to comment #42)

> > This part bothers me. What API do you keep in mind taking about show/hide
> > events?
> 
> MSAA: EVENT_OBJECT_SHOW, EVENT_OBJECT_HIDE
> ATK: children_changed::add / remove
> 
> > Since Gecko's show/hide events are result of tree changes then should
> > we change the tree?
> 
> No.

Why do we want AT to change the tree but the same time we don't want to change it on our side? Sounds like a cheating.

    
    

    
  
Btw, did you say recently that you ARIA groups gets agreed that aria-hidden="true" on visible content doesn't hide the tree? Is that's correct then no tree changes for this case.

    
    

    
  
Comment on attachment 504491 [details] [diff] [review]
patch 1

(In reply to comment #42)

> > Since Gecko's show/hide events are result of tree changes then should
> > we change the tree?
> 
> No.

hide event processing changes the tree.

perhaps it makes sense to get a patch without show/hide suspect things to work on them separately?

        Attachment #504491 -
        Flags: review?(surkov.alexander) → review-

    
    

    
  
(In reply to comment #47)
> Comment on attachment 504491 [details] [diff] [review]
> patch 1
> 
> (In reply to comment #42)
> 
> > > Since Gecko's show/hide events are result of tree changes then should
> > > we change the tree?
> > 
> > No.
> 
> hide event processing changes the tree.
> 
> perhaps it makes sense to get a patch without show/hide suspect things to work
> on them separately?

I recall worrying initially about the hide event when I was writing the patch but for some reason I figured it was safe but we are late in the game so I'm fine with doing show/hide on a separate bug.

    
    

    
  
Filed bug 626642 for show/hide/other.
Summary: aria-hidden should fire attributechange and show/hide events → aria-hidden should fire attributechange and expose visibility

    
    

    
  


  
Having a specific aria-hidden test file might seem overkill, but we might as well in anticipation of bug 626642 work.

        Attachment #504491 -
        Attachment is obsolete: true

        Attachment #504769 -
        Flags: review?(surkov.alexander)

    
    

    
  
Comment on attachment 504769 [details] [diff] [review]
patch 1.1 (removed show/hide eventing)


>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -1235,17 +1235,17 @@ nsAccessibilityService::HasUniversalAria
>          nsAccUtils::HasDefinedARIAToken(aContent, nsAccessibilityAtoms::aria_busy) ||
>          aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_controls) ||
>          aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_describedby) ||
>          aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_disabled) ||
>          nsAccUtils::HasDefinedARIAToken(aContent, nsAccessibilityAtoms::aria_dropeffect) ||
>          aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_flowto) ||
>          nsAccUtils::HasDefinedARIAToken(aContent, nsAccessibilityAtoms::aria_grabbed) ||
>          nsAccUtils::HasDefinedARIAToken(aContent, nsAccessibilityAtoms::aria_haspopup) ||
>-         // purposely ignore aria-hidden; since we use gecko for detecting this anyways
>+         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_hidden) ||

contradicts with "the element must still be exposed if it has a WAI-ARIA global attribute other than aria-hidden" of ARIA impl guide.

    
    

    
  
(In reply to comment #51)

> contradicts with "the element must still be exposed if it has a WAI-ARIA global
> attribute other than aria-hidden" of ARIA impl guide.

ok, no contradiction, especially with clarification:

User agents MAY provide an accessible object in the accessible tree corresponding to DOM elements that meet the following criteria:

    has a WAI-ARIA global attribute of aria-hidden="true". 

but I still don't understand why do you want to expose it when it shouldn't be by default

    
    

    
  
(In reply to comment #51)
> Comment on attachment 504769 [details] [diff] [review]
> patch 1.1 (removed show/hide eventing)


> >-         // purposely ignore aria-hidden; since we use gecko for detecting this anyways
> >+         aContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_hidden) ||
> 
> contradicts with "the element must still be exposed if it has a WAI-ARIA global
> attribute other than aria-hidden" of ARIA impl guide.

No, that's obsolete spec language, it was when pruning the tree was a MUST.

    
    

    
  
(In reply to comment #52)
> (In reply to comment #51)

> but I still don't understand why do you want to expose it when it shouldn't be
> by default

Ditto. That language will be removed or qualified in the spec.

    
    

    
  
I still don't get a reason why do you need an accessible for, for example,
<span aria-hidden="true"></span>?

    
    

    
  
(In reply to comment #55)
> I still don't get a reason why do you need an accessible for, for example,
> <span aria-hidden="true"></span>?

Sure makes implementation simple, and seems harmless to me.

    
    

    
  
Comment on attachment 504769 [details] [diff] [review]
patch 1.1 (removed show/hide eventing)

>diff --git a/accessible/src/base/nsARIAMap.cpp b/accessible/src/base/nsARIAMap.cpp
>--- a/accessible/src/base/nsARIAMap.cpp
>+++ b/accessible/src/base/nsARIAMap.cpp
>@@ -634,16 +634,20 @@ nsStateMapEntry nsARIAMap::gWAIStateMap[
>   nsStateMapEntry(&nsAccessibilityAtoms::aria_expanded, kBoolType, 0,
>                   nsIAccessibleStates::STATE_EXPANDED, 0,
>                   nsIAccessibleStates::STATE_COLLAPSED, 0),
> 
>   // eARIAHasPopup
>   nsStateMapEntry(&nsAccessibilityAtoms::aria_haspopup, kBoolType, 0,
>                   nsIAccessibleStates::STATE_HASPOPUP, 0),
> 
>+  // eARIAHidden
>+  nsStateMapEntry(&nsAccessibilityAtoms::aria_hidden, kBoolType, 0,
>+                  nsIAccessibleStates::STATE_INVISIBLE, 0),

it sounds like it's not supposed to be propagated to children but it contradicts with spec "Indicates that the element and all of its descendants are not visible". Is it supposed to be changed as well?

Why do you need state invisible if you expose object attribute and fire object attribute change but do not fire state change event?

    
    

    
  
(In reply to comment #56)
> (In reply to comment #55)
> > I still don't get a reason why do you need an accessible for, for example,
> > <span aria-hidden="true"></span>?
> 
> Sure makes implementation simple, and seems harmless to me.

It appears you don't simplify implementation by adding new checks. While you don't propagate invisible state then what's a reason to expose node that invisible and will be ignored by AT?

    
    

    
  
(In reply to comment #57)
> Comment on attachment 504769 [details] [diff] [review]
> patch 1.1 (removed show/hide eventing)


> >+  // eARIAHidden
> >+  nsStateMapEntry(&nsAccessibilityAtoms::aria_hidden, kBoolType, 0,
> >+                  nsIAccessibleStates::STATE_INVISIBLE, 0),
> 
> it sounds like it's not supposed to be propagated to children but it
> contradicts with spec "Indicates that the element and all of its descendants
> are not visible". Is it supposed to be changed as well?

I don't want to take that road.
 
> Why do you need state invisible if you expose object attribute and fire object
> attribute change but do not fire state change event?

Good question!

    
    

    
  
(In reply to comment #58)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > I still don't get a reason why do you need an accessible for, for example,
> > > <span aria-hidden="true"></span>?
> > 
> > Sure makes implementation simple, and seems harmless to me.
> 
> It appears you don't simplify implementation by adding new checks.

Which checks don't you like?

> While you
> don't propagate invisible state then what's a reason to expose node that
> invisible and will be ignored by AT?

If I wrote an AT I wouldn't ignore an object with a 'hidden' attribute.

    
    

    
  
(In reply to comment #59)
> (In reply to comment #57)
> > Comment on attachment 504769 [details] [diff] [review]
> > patch 1.1 (removed show/hide eventing)
> 
> 
> > >+  // eARIAHidden
> > >+  nsStateMapEntry(&nsAccessibilityAtoms::aria_hidden, kBoolType, 0,
> > >+                  nsIAccessibleStates::STATE_INVISIBLE, 0),
> > 
> > it sounds like it's not supposed to be propagated to children but it
> > contradicts with spec "Indicates that the element and all of its descendants
> > are not visible". Is it supposed to be changed as well?
> 
> I don't want to take that road.

To explain further. I don't want to implement propagation of aria-hidden induced visibility state because it isn't clear it is needed. I'd rather the AT track hidden object attribute changes and decide for itself.

    
    

    
  
Note, I would be fine with pulling the visibility state out of this patch. I think we need to at least land the attribute change for FF4.

    
    

    
  
BTW I sent the spec change request (re: comment 53 and comment 54) to the editor.

        Attachment #504769 -
        Flags: review?(surkov.alexander)

    
    

    
  
If we do 1) expose aria-hidden as object attribute 2) fire object attribute change event 3) create an accessible for element that is not accessible by default but that has aria-hidden attribute then it looks plain and fine with me. I don't like visibility state mapping because it's smelly and not clear. Are you fine to file for that separate bug?

    
    

    
  
Then could you please file new patch? :)

    
    

    
  


  
(In reply to comment #65)
> Then could you please file new patch? :)

But of course :)

        Attachment #504769 -
        Attachment is obsolete: true

        Attachment #504804 -
        Flags: review?(surkov.alexander)

    
    

    
  
For the record, I disagree with doing this using an object attribute. If aria-hidden is true, the author clearly intended this node to be hidden (a.k.a. invisible) to ATs.

To be honest, I don't see the need for aria-hidden at all, especially if it doesn't hide descendants. I'm not even sure we can support exposing a node's descendants without exposing the node itself in NVDA without a huge amount of work.

    
    

    
  
(In reply to comment #67)
> To be honest, I don't see the need for aria-hidden at all, especially if it
> doesn't hide descendants. I'm not even sure we can support exposing a node's
> descendants without exposing the node itself in NVDA without a huge amount of
> work.

I'm not100% sure what you mean. Just to be clear, aria-hidden is meant to indicate that the whole subtree is not meant to be exposed to AT.

I think we need to land the minimal patch and move on. This bug is a bit of a sink hole.

    
    

    
  
(In reply to comment #68)
> Just to be clear, aria-hidden is meant to
> indicate that the whole subtree is not meant to be exposed to AT.
Ah. My bad. I misunderstood your comments about not propagating the hidden state to descendants.

(In reply to comment #61)
> To explain further. I don't want to implement propagation of aria-hidden
> induced visibility state because it isn't clear it is needed. I'd rather the AT
> track hidden object attribute changes and decide for itself.
This is okay for our buffer where we crawl the entire hierarchy, but it is problematic for cases where the entire hierarchy isn't traversed. For example, object navigation accesses each object in turn. In order to know whether a given object is hidden, we would have to crawl up the entire hierarchy looking for the hidden attribute.

(In reply to comment #68)
> I think we need to land the minimal patch and move on. This bug is a bit of a
> sink hole.
I understand; this is certainly not critical relative to other issues that need fixing for FF4. However, it's possible we will choose not to support this initial implementation in NVDA; it is too incomplete imo.

    
    

    
  
(In reply to comment #69)
> (In reply to comment #61)
> > To explain further. I don't want to implement propagation of aria-hidden
> > induced visibility state because it isn't clear it is needed. I'd rather the AT
> > track hidden object attribute changes and decide for itself.
> This is okay for our buffer where we crawl the entire hierarchy, but it is
> problematic for cases where the entire hierarchy isn't traversed. For example,
> object navigation accesses each object in turn. In order to know whether a
> given object is hidden, we would have to crawl up the entire hierarchy looking
> for the hidden attribute.

Yeah, and you don't cache anything for object navigation right? (Otherwise you could watch the aria-hidden attribute changes and update cached states).

> 
> (In reply to comment #68)
> > I think we need to land the minimal patch and move on. This bug is a bit of a
> > sink hole.
> I understand; this is certainly not critical relative to other issues that need
> fixing for FF4. However, it's possible we will choose not to support this
> initial implementation in NVDA; it is too incomplete imo.

That's reasonable. At least the door is open, even if not ideal.

    
    

    
  
(In reply to comment #70)
> > > I don't want to implement propagation of aria-hidden
> > > induced visibility state
> > This is ... problematic for cases where the entire hierarchy isn't traversed. For example,
> > object navigation accesses each object in turn. In order to know whether a
> > given object is hidden, we would have to crawl up the entire hierarchy looking
> > for the hidden attribute.
> Yeah, and you don't cache anything for object navigation right? (Otherwise you
> could watch the aria-hidden attribute changes and update cached states).
We don't cache anything for objects that aren't currently of interest; i.e. unless it is the focus, an ancestor of the focus or the current review object. Even if it *was* one of those objects, child objects would still have to crawl up the hierarchy to work out whether they are hidden and would probably pass through several uncached objects.

    
    

    
  
Do we really need to expose aria-hidden as object attribute per Jamie's comment 67? If AT that don't deal with DOM like NVDA don't need it then we shouldn't expose it as object attribute. DOM-based AT use ISimpleDOMNode and presumably don't need IA2 for that.

For the record no one of patches propagates aria-hidden to subtree, neither as object attribute nor as invisible state (previous patch).

    
    

    
  
(In reply to comment #72)
> Do we really need to expose aria-hidden as object attribute

We already do expose it as an object attribute. (We expose all aria-foo as attributes unless we explicitly bypass them). My wimpy patch just makes not bypassing aria-hidden explicit and adds an object attribute changed event for it should an AT find it useful to watch for it. I understand the pros and cons of the various ideas presented in this bug and I think this is a fair compromise. I certainly don't want to run around in circles for aria-hidden anymore.

    
    

    
  
(In reply to comment #73)
> (In reply to comment #72)
> > Do we really need to expose aria-hidden as object attribute
> 
> I understand the pros and cons
> of the various ideas presented in this bug and I think this is a fair
> compromise. I certainly don't want to run around in circles for aria-hidden
> anymore.

The concerns is perhaps we add a code that isn't going to be used by somebody. That makes me doubt. I'm fine to follow object approach (comment #64). But then if we're going to map aria-hidden to invisible states then object attribute change might be excess (in the case of state change event). Note, we add these changes in the end of Firefox 4 release and all temporary things will hang for a long time. I really would like to have complete proposition if possible. Or some thoughts how AT are going to handle it.

So I'd ask two questions. Are we going to destroy or hide accessible tree from AT at all? And if yes how are we going to deal with focus inside of destroyed or hidden accessible subtree?

    
    

    
  
(In reply to comment #74)
> So I'd ask two questions. Are we going to destroy or hide accessible tree from
> AT at all?

We decided 'no' already at least for FF4.

    
    

    
  
Jamie, thanks for your input on this bug. We listened. Ultimately we are not comfortable with the hairy mess that pretending a tree is invisible or removed might cause, especially when considering focus might go into the invisible tree[1] etc. This is the fault of the way aria-hidden is now defined. Which is not as originally intended, namely, as a hint for DOM based AT. What we are doing here in the latest patch is the IA2 equivalent of that original idea.

[1]While there are ideas about how to deal with that, it introduces risk we can't take for FF4.

    
    

    
  
Comment on attachment 504804 [details] [diff] [review]
patch 1.2 (only obj attr)

Ok, it allow IA2 AT to deal with it how they want. It appears they may not want to deal with it at all so that we're doing excess work. Since there's no total agreement on the right way how to expose aria-hidden then let's take the patch even it's useless in the end.

> 
>   // For aria drag and drop changes we fire a generic attribute change event;
>   // at least until native API comes up with a more meaningful event.
>+  // XXX Bug 626642 provide better events for aria-hidden (show/hide?)

no XXX, otherwise I feel like we're doing something wrong.

>+    function hideNode(aID, bHide)
>+    {
>+      this.node = getNode(aID);
>+      this.accessible =
>+        isAccessible(this.node) ? getAccessible(this.node) : null;

what is this? It must be accessible. It's better failure than infinite exception for the event targeted to null.

please add a test into attributes/test_obj.html

        Attachment #504804 -
        Flags: review?(surkov.alexander) → review+

    
    

    
  
(In reply to comment #77)
> Comment on attachment 504804 [details] [diff] [review]

> >+  // XXX Bug 626642 provide better events for aria-hidden (show/hide?)
> 
> no XXX, otherwise I feel like we're doing something wrong.

Done.

> >+      this.accessible =
> >+        isAccessible(this.node) ? getAccessible(this.node) : null;
> 
> what is this? It must be accessible. It's better failure than infinite
> exception for the event targeted to null.

Copy and paste... not necessary you are right.

> please add a test into attributes/test_obj.html

Sure.

    
    

    
  
(In reply to comment #76)
> Jamie, thanks for your input on this bug. We listened. Ultimately we are not
> comfortable with the hairy mess that pretending a tree is invisible or removed
> might cause, especially when considering focus might go into the invisible
> tree[1] etc.
Fair enough; thanks for listening. As I said in comment #69, I understand your position. I'm very familiar with the difficulties associated with release decisions. :)

> This is the fault of the way aria-hidden is now defined. Which is
> not as originally intended, namely, as a hint for DOM based AT.
Personally, I think aria-hidden is a terrible idea. In my experience, such things are generally misused and cause far too much trouble. It doesn't even make sense as a hint for DOM based ATs; this could lead to inconsistency. 

> What we are
> doing here in the latest patch is the IA2 equivalent of that original idea.
This does give us the ability to handle it if we choose to. However, we came to the conclusion a while ago that there are certain aspects of HTML/ARIA that we will not support due to the fact that they cause more problems than they solve for our users. I suspect aria-hidden will be one of these.
Summary: aria-hidden should fire attributechange and expose visibility → aria-hidden should expose hidden attribute and fire attributechange events

    
    

    
  
landed on 2.0 beta 11 - http://hg.mozilla.org/mozilla-central/rev/d55f25baadb3
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: