814836 - <xul:deck> element messes up screen readers

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Wladimir Palant]

Attachment: Testcase

It seems that Firefox is reporting wrong structure for XUL documents to screen readers if a <xul:deck> element is present. I attached a testcase, it needs to be downloaded and added to some chrome namespace (won't open otherwise due to remote XUL restrictions). That testcase shows four strings:

test1
test2
test3
test4

The second and third string are inside a <deck> element, the others are just regular <description> elements. A screen reader should normally read all of them nevertheless. However, when I test it with a Firefox 20.0a1 nightly (2012-11-23) on Linux with the Orca screen reader only test1 and test4 are being read. With more complicated examples I get more complicated failures, e.g. in the Filter Preferences dialog of Adblock Plus 2.2.1 (<richlistbox> with items containing a <deck> along with several other elements) some elements inside the items are being read when the dialog window opens and others when the selection changes - I couldn't figure out the system behind it. It all stops as soon as I remove the <deck> element.

This is apparently a cross-platform issue, the original report referred to JAWS which is Windows-only. And it is a regression, the regression range I found is:

Last good nightly: 2012-07-06
First bad nightly: 2012-07-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4b1249ae1906&tochange=1751d97cc9e4

Unfortunately, I couldn't find any obvious candidates in the push log, accessibility changes seems to touch only Windows and Mac specific code.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to Wladimir Palant from comment #0)

> It seems that Firefox is reporting wrong structure for XUL documents to
> screen readers if a <xul:deck> element is present. 
> The second and third string are inside a <deck> element, the others are just
> regular <description> elements. A screen reader should normally read all of
> them nevertheless. However, when I test it with a Firefox 20.0a1 nightly
> (2012-11-23) on Linux with the Orca screen reader only test1 and test4 are
> being read.

Did you compare hierarchy between builds just in case? (DOMi, Accerciser)

> With more complicated examples I get more complicated failures,
> e.g. in the Filter Preferences dialog of Adblock Plus 2.2.1 (<richlistbox>
> with items containing a <deck> along with several other elements) 

That may be different issue, AT might not expect to see deck inside listboxes.

> This is apparently a cross-platform issue, the original report referred to
> JAWS which is Windows-only.

That's interesting. Afaik screen readers feel well with Option Dialog which uses deck element as well.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #1)

> That's interesting. Afaik screen readers feel well with Option Dialog which
> uses deck element as well.

sorry, About Dialog

Comment 3

[Wladimir Palant]

(In reply to alexander :surkov from comment #1)
> Did you compare hierarchy between builds just in case? (DOMi, Accerciser)

No, I forgot that DOMi could show accessibility nodes - never used that feature before.

> That may be different issue, AT might not expect to see deck inside
> listboxes.

I would also suspect that it is a different issue - but it has exactly the same regression range.

Comment 4

[alexander :surkov (:asurkov)]

the difference between those builds: 06 June's one doesn't have an accessible for XUL deck. Extra grouping role (MSAA role) shouldn't confuse JAWS, perhaps pane role (ATK role) might confuse Orca. Extra grouping accessible inside listbox can confuse JAWS I guess.

In bug 810260 I guessed that we don't need an accessible for xul:deck in general so might be it's time to get rid it.

Comment 5

[alexander :surkov (:asurkov)]

and then in bug bug 750612 we "legalized" an accessible xul:deck

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

1) Do not create an accessible for deck (partial back out of bug 750612). As a rule deck is not part of UI and used to show/hide UI elements (analogue of HTML display style technique), for example, it's used inside listboxes for that propose.
2) Do not create an accessible for deck panel (partial backout of bug 357969). Tabpanels have semantics, deck panels don't.
3) Do not create accessibles for children of not selected deck panel (fall into usual tree update procedure when selected panel is changed).
4) Do not touch tabpanels.

3 and 4 guarantee we don't regress bug 750612.
4 guarantees us we don't regress bug 357969.

Note, Firefox option dialog uses deck for top level tabs (like 'general', 'tabs' and etc) but those tabpanels aren't connected with those buttons. It doesn't seem we regress user experience on this.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 689288 [details] [diff] [review]
patch2

>diff --git a/accessible/public/nsIAccessibleProvider.idl b/accessible/public/nsIAccessibleProvider.idl
>--- a/accessible/public/nsIAccessibleProvider.idl
>+++ b/accessible/public/nsIAccessibleProvider.idl
>@@ -57,18 +57,18 @@ interface nsIAccessibleProvider : nsISup
>   const long XULStatusBar = 0x00001014;
>   const long XULRadioButton = 0x00001015;
>   const long XULRadioGroup = 0x00001016;
> 
>   /** Used for XUL tab element */
>   const long XULTab = 0x00001017;
>   /** Used for XUL tabs element, a container for tab elements */
>   const long XULTabs = 0x00001018;
>-  /** Used for XUL deck frame */
>-  const long XULDeck = 0x00001019;
>+  /** Used for XUL tabpanels element */
>+  const long XULTabpanels = 0x00001019;
> 
>   const long XULText             = 0x0000101A;
>   const long XULTextBox          = 0x0000101B;
>   const long XULThumb            = 0x0000101C;
>   const long XULTree             = 0x0000101D;
>   const long XULTreeColumns      = 0x0000101E;
>   const long XULTreeColumnItem   = 0x0000101F;
>   const long XULToolbar          = 0x00001020;
>diff --git a/accessible/src/base/AccTypes.h b/accessible/src/base/AccTypes.h
>--- a/accessible/src/base/AccTypes.h
>+++ b/accessible/src/base/AccTypes.h
>@@ -47,17 +47,17 @@ enum AccType {
>   /**
>    * Other accessible types.
>    */
>   eApplication = 25,
>   eImageMap = 26,
>   eMenuPopup = 27,
>   eProgress = 28,
>   eRoot = 29,
>-  eXULDeck = 30,
T>+  eXULTabpanels = 30,
>   eXULTree = 31
> };
> 
> /**
>  * Generic accessible type, different accessible classes can share the same
>  * type, the same accessible class can have several types.
>  */
> enum AccGenericType {
>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -54,16 +54,17 @@
> #include "nsIObserverService.h"
> #include "nsLayoutUtils.h"
> #include "nsNPAPIPluginInstance.h"
> #include "nsObjectFrame.h"
> #include "mozilla/dom/Element.h"
> #include "mozilla/Preferences.h"
> #include "mozilla/Services.h"
> #include "mozilla/Util.h"
>+#include "nsDeckFrame.h"
> 
> #ifdef MOZ_XUL
> #include "XULAlertAccessible.h"
> #include "XULColorPickerAccessible.h"
> #include "XULComboboxAccessible.h"
> #include "XULElementAccessibles.h"
> #include "XULFormControlAccessible.h"
> #include "XULListboxAccessibleWrap.h"
>@@ -249,16 +250,57 @@ nsAccessibilityService::CreatePluginAcce
> #endif
>   }
> #endif
> 
>   return nullptr;
> }
> 
> void
>+nsAccessibilityService::DeckPanelSwitched(nsIPresShell* aPresShell,
>+                                          nsIContent* aDeckNode,
>+                                          nsIFrame* aPrevBoxFrame,
>+                                          nsIFrame* aCurrentBoxFrame)
>+{

I think I'd prefer this was a method on DocAccessible

>+  // Ignore tabpanels elements (a deck having an accessible) since their
>+  // children are accessible not depending on selected tab.
>+  DocAccessible* document = GetDocAccessible(aPresShell);

why not just do aPresShell->GetDocAccessible() we know the pres shell is non null so we don't need the branching in that method.

>+    if (!newAcc && aContext->IsXULTabpanels() &&
>+      content->GetParent() == aContext->GetContent() &&
>+      (frame->GetType() == nsGkAtoms::boxFrame ||
>+        frame->GetType() == nsGkAtoms::scrollFrame)) {

I wonder if it would be a good idea to put the result of frame->GetType() in a variable since its virtual (I'm not hope the compiler is smart enough to do it itself but...)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> >+nsAccessibilityService::DeckPanelSwitched(nsIPresShell* aPresShell,
> >+                                          nsIContent* aDeckNode,
> >+                                          nsIFrame* aPrevBoxFrame,
> >+                                          nsIFrame* aCurrentBoxFrame)
> >+{
> 
> I think I'd prefer this was a method on DocAccessible

I would leave that to the bug we have about moving the methods out of the service.


> why not just do aPresShell->GetDocAccessible() we know the pres shell is non
> null so we don't need the branching in that method.

technically they have a difference. I think I'd like to keep notifications the same way, i.e. they are allowed to trigger document creation.

> I wonder if it would be a good idea to put the result of frame->GetType() in
> a variable since its virtual (I'm not hope the compiler is smart enough to
> do it itself but...)

ok

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > >+nsAccessibilityService::DeckPanelSwitched(nsIPresShell* aPresShell,
> > >+                                          nsIContent* aDeckNode,
> > >+                                          nsIFrame* aPrevBoxFrame,
> > >+                                          nsIFrame* aCurrentBoxFrame)
> > >+{
> > 
> > I think I'd prefer this was a method on DocAccessible
> 
> I would leave that to the bug we have about moving the methods out of the
> service.

only because you like to keep them here, but I guess that bug is a better place to convince you ;/)

> > why not just do aPresShell->GetDocAccessible() we know the pres shell is non
> > null so we don't need the branching in that method.
> 
> technically they have a difference. I think I'd like to keep notifications
> the same way, i.e. they are allowed to trigger document creation.

they could in theory sure, but is it actually possible? if doc existed before a11y starts then code in ApplicationAccessible::CacheChildren() should create it otherwise normal doc creation logic should no?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > I would leave that to the bug we have about moving the methods out of the
> > service.
> 
> only because you like to keep them here, but I guess that bug is a better
> place to convince you ;/)

you're right, I'm no sure what is best

> > > why not just do aPresShell->GetDocAccessible() we know the pres shell is non
> > > null so we don't need the branching in that method.
> > 
> > technically they have a difference. I think I'd like to keep notifications
> > the same way, i.e. they are allowed to trigger document creation.
> 
> they could in theory sure, but is it actually possible? if doc existed
> before a11y starts then code in ApplicationAccessible::CacheChildren()
> should create it otherwise normal doc creation logic should no?

true, AppAcc creates root accessibles and its trees, OuterDocAccessible creates underlying documents and so on. But I really would prefer to change them all together separately. If you like I can file a bug.

Comment 12

[Trevor Saunders (:tbsaunde)]

> > > > why not just do aPresShell->GetDocAccessible() we know the pres shell is non
> > > > null so we don't need the branching in that method.
> > > 
> > > technically they have a difference. I think I'd like to keep notifications
> > > the same way, i.e. they are allowed to trigger document creation.
> > 
> > they could in theory sure, but is it actually possible? if doc existed
> > before a11y starts then code in ApplicationAccessible::CacheChildren()
> > should create it otherwise normal doc creation logic should no?
> 
> true, AppAcc creates root accessibles and its trees, OuterDocAccessible
> creates underlying documents and so on. But I really would prefer to change
> them all together separately. If you like I can file a bug.

I'm not sure I see a point in doing them all together, but sure file a bug if you prefer.

Comment 13

[alexander :surkov (:asurkov)]

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

there's a small risk, if we missed something, and since these are unrelated things then I prefer to deal with them separately.

Comment 14

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c3756c0705

Comment 15

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/57c3756c0705