396166 - Accessible <xul:label value="foo"> should implement nsIAccessibleText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

If you use <xul:label value> to set a label (instead of child content), then the accessible label object that's created does not implement the accessible text interface. It should. In addition, we don't fire text change events when it changes. 

This may also be a problem for things like <checkbox label="foo">. Basically, anything that uses the nsTextBoxFrame.

Fortunately we do expose the text as the accessible name, and right now Orca is falling back on that when there is no accessible text.

* Reason for problem: *

The value attribute doesn't appear as an anonymous child of the xul:label.  The XUL label is implemented with nsTextBoxFrame.cpp. There is a string called
mTitle and mCroppedTitle. The PaintTitle() method is where the label is
painted.

* Ideas for implementing a fix: *

I can think of 3 ways to fix this:
1) Reimplement <label value> using an anonymous XBL child text node instead --
big risky change, not for FF3 that's for sure
2) Change nsHyperTextAccessible so it can get text from the accessible name of
the label, similar to how list bullets currently get their text from the
accessible name -- many methods such as character extents won't work
3) Implement nsIAccessibleText directly on nsXULTextAccessible -- lots of work

Comment 1

[alexander :surkov (:asurkov)]

the same problem must exists for xul:description@value too.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Mass un-assigning bugs assigned to Aaron.

Comment 4

[alexander :surkov (:asurkov)]

update on way to fix:

1) nowdays xul label and description doesn't have XBL anonymous children (see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/text.xml#29), I assume the native anonymous text node is created for text specified by @value attribute (though I'm not sure why we don't expose it then)
2) I don't like to heavy nsHyperTextAccessible class by specific cases
3) Way to go (if 1. doesn't work due to some reason) but we need to rethink nsHyperTextAccessible impl to have a few overrides in nsXULTextAccessible

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

2nd approach of the bug description

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 716400 [details] [diff] [review]
patch

>+nsAccessibilityService::UpdateLabelValue(nsIPresShell* aPresShell,
>+                                         nsIContent* aLabelElm)
>+{
>+  DocAccessible* document = GetDocAccessible(aPresShell);
>+  if (document) {

can we assert there's always a doc accessible?

>+    Accessible* accessible = document->GetAccessible(aLabelElm);
>+    if (accessible) {
>+      XULLabelAccessible* xulLabel = accessible->AsXULLabel();
>+      if (xulLabel)

same for these

> XULLabelAccessible::
>   XULLabelAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>   HyperTextAccessibleWrap(aContent, aDoc)
> {
>+  mType = eXULLabelType;
>+
>+  // If @value attribute is given then it's rendered instead text content. In
>+  // this case we need to create a text leaf accessible to make @value attribute
>+  // accessible.
>+  // XXX: text interface doesn't let you get the text by words.
>+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
>+  if (textBoxFrame) {
>+    mValueTextLeaf = new XULLabelValueAccessible(mContent, mDoc);

why can't we stuff this in mChildren[0] ?

> XULLabelAccessible::NativeName(nsString& aName)
> {
>   // if the value attr doesn't exist, the screen reader must get the accessible text
>   // from the accessible text interface or from the children
>-  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, aName);
>+  if (mValueTextLeaf)
>+    return mValueTextLeaf->Name(aName);

if you just use mChildren[0] then you need to distinguish if that's the special XULLabelValue thing so why don't we allow sub classes to use one of the bits  in Accessible's bit field after mGeneric type for there own purposes?  I'm thinking I'll want that for bug 809338 too.

>+XULLabelAccessible::UpdateLabelValue()
>+{
>+  NS_PRECONDITION(mValueTextLeaf, "Wrong usage of UpdateLabelValue!");
>+  mValueTextLeaf->UpdateText();

inline it?

>+  XULLabelValueAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>+  LeafAccessible(aContent, aDoc)
>+{
>+  mStateFlags |= eSharedNode;
>+
>+  nsTextBoxFrame* frame = do_QueryFrame(mContent->GetPrimaryFrame());
>+  NS_ASSERTION(frame, "Wrong usage of XULLabelValueAccessible!");
>+  if (frame)

that's just plain impossible right? so why don't we get rid of the check?  or better, pass the frame as an arg to the constructor.

>+XULLabelValueAccessible::UpdateText()
>+{
>+  nsAutoString text;
>+  nsTextBoxFrame* frame = do_QueryFrame(mContent->GetPrimaryFrame());
>+  NS_ASSERTION(frame, "Wrong usage of XULLabelValueAccessible!");
>+  if (frame)
>+    frame->GetCroppedTitle(text);

why don't you just have layout pass the string to the nsAccessibilityService method as an argument, then you don't need the do_QueryInterface() and stuff here

>+class XULLabelValueAccessible : public LeafAccessible

final?

>+      this.finalCheck = function setCrop_finalCheck()
>+      {
>+        getNode("textbox").value = getAccessible(this.labelNode, [nsIAccessibleText]).getText(0, -1);

don't you want to be checking those are the same?

I really think we should also have tests for the imlemenetation of the text api on these elements.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from c
> >+nsAccessibilityService::UpdateLabelValue(nsIPresShell* aPresShell,
> >+                                         nsIContent* aLabelElm)
> >+{
> >+  DocAccessible* document = GetDocAccessible(aPresShell);
> can we assert there's always a doc accessible?

won't we assert for things like hidden windows?

> >+    Accessible* accessible = document->GetAccessible(aLabelElm);
> >+    if (accessible) {
> >+      XULLabelAccessible* xulLabel = accessible->AsXULLabel();
> >+      if (xulLabel)
> 
> same for these

ok

> > XULLabelAccessible::
> >   XULLabelAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> >+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> >+  if (textBoxFrame) {
> >+    mValueTextLeaf = new XULLabelValueAccessible(mContent, mDoc);
> 
> why can't we stuff this in mChildren[0] ?

we do that on CacheChildren(), that's the place were we build children.

> > XULLabelAccessible::NativeName(nsString& aName)
> > {
> >   // if the value attr doesn't exist, the screen reader must get the accessible text
> >   // from the accessible text interface or from the children
> >-  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, aName);
> >+  if (mValueTextLeaf)
> >+    return mValueTextLeaf->Name(aName);
> 
> if you just use mChildren[0] then you need to distinguish if that's the
> special XULLabelValue thing so why don't we allow sub classes to use one of
> the bits  in Accessible's bit field after mGeneric type for there own
> purposes?  I'm thinking I'll want that for bug 809338 too.

I'm not sure I follow

> >+XULLabelAccessible::UpdateLabelValue()
> >+{
> >+  NS_PRECONDITION(mValueTextLeaf, "Wrong usage of UpdateLabelValue!");
> >+  mValueTextLeaf->UpdateText();
> 
> inline it?

add -inl.h file or move up XULLabelValueAccessible class in header?

> >+  XULLabelValueAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> >+  LeafAccessible(aContent, aDoc)
> >+{
> >+  mStateFlags |= eSharedNode;
> >+
> >+  nsTextBoxFrame* frame = do_QueryFrame(mContent->GetPrimaryFrame());
> >+  NS_ASSERTION(frame, "Wrong usage of XULLabelValueAccessible!");
> >+  if (frame)
> 
> that's just plain impossible right? so why don't we get rid of the check? 
> or better, pass the frame as an arg to the constructor.

sounds good

> >+XULLabelValueAccessible::UpdateText()

you know I think to rename it to XULLabelTextLeafAccessible, it seems more accurate.

> >+{
> >+  nsAutoString text;
> >+  nsTextBoxFrame* frame = do_QueryFrame(mContent->GetPrimaryFrame());
> >+  NS_ASSERTION(frame, "Wrong usage of XULLabelValueAccessible!");
> >+  if (frame)
> >+    frame->GetCroppedTitle(text);
> 
> why don't you just have layout pass the string to the nsAccessibilityService
> method as an argument, then you don't need the do_QueryInterface() and stuff
> here

ok

> >+class XULLabelValueAccessible : public LeafAccessible
> 
> final?

ok

> >+      this.finalCheck = function setCrop_finalCheck()
> >+      {
> >+        getNode("textbox").value = getAccessible(this.labelNode, [nsIAccessibleText]).getText(0, -1);
> 
> don't you want to be checking those are the same?

it's a pure debugging :)

> I really think we should also have tests for the imlemenetation of the text
> api on these elements.

that'd be good, I can add some

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from c
> > >+nsAccessibilityService::UpdateLabelValue(nsIPresShell* aPresShell,
> > >+                                         nsIContent* aLabelElm)
> > >+{
> > >+  DocAccessible* document = GetDocAccessible(aPresShell);
> > can we assert there's always a doc accessible?
> 
> won't we assert for things like hidden windows?

well, it only gets called if the window is being layed out and the  content has a frame.  So I would assert that either such windows shouldn't get layed out or they should have accessibles.

> > >   XULLabelAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> > >+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> > >+  if (textBoxFrame) {
> > >+    mValueTextLeaf = new XULLabelValueAccessible(mContent, mDoc);
> > 
> > why can't we stuff this in mChildren[0] ?
> 
> we do that on CacheChildren(), that's the place were we build children.

I was suggesting to get rid of seperate member.

> > >+XULLabelAccessible::UpdateLabelValue()
> > >+{
> > >+  NS_PRECONDITION(mValueTextLeaf, "Wrong usage of UpdateLabelValue!");
> > >+  mValueTextLeaf->UpdateText();
> > 
> > inline it?
> 
> add -inl.h file or move up XULLabelValueAccessible class in header?

I guess I'd just reorder the classes, I see no reason not to put text leaf thing first

> you know I think to rename it to XULLabelTextLeafAccessible, it seems more
> accurate.

/me shrugs

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> (In reply to alexander :surkov from comment #7)
> > (In reply to Trevor Saunders (:tbsaunde) from c
> > > >+nsAccessibilityService::UpdateLabelValue(nsIPresShell* aPresShell,
> > > >+                                         nsIContent* aLabelElm)
> > > >+{
> > > >+  DocAccessible* document = GetDocAccessible(aPresShell);
> > > can we assert there's always a doc accessible?
> > 
> > won't we assert for things like hidden windows?
> 
> well, it only gets called if the window is being layed out and the  content
> has a frame.  So I would assert that either such windows shouldn't get layed
> out or they should have accessibles.

per bug 794041 comment #8 hidden windows have frame tree. Do you mean that method isn't trigger because things aren't drawn?

> > > >   XULLabelAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> > > >+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> > > >+  if (textBoxFrame) {
> > > >+    mValueTextLeaf = new XULLabelValueAccessible(mContent, mDoc);
> > > 
> > > why can't we stuff this in mChildren[0] ?
> > 
> > we do that on CacheChildren(), that's the place were we build children.
> 
> I was suggesting to get rid of seperate member.

we still have InvalidateChildren(), I think I wouldn't try something new here.

> > add -inl.h file or move up XULLabelValueAccessible class in header?
> 
> I guess I'd just reorder the classes, I see no reason not to put text leaf
> thing first

ok

> 
> > you know I think to rename it to XULLabelTextLeafAccessible, it seems more
> > accurate.
> 
> /me shrugs

ok

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 11

[Trevor Saunders (:tbsaunde)]

> > I really think we should also have tests for the imlemenetation of the text
> > api on these elements.
> 
> that'd be good, I can add some

what about this?

> we still have InvalidateChildren(), I think I wouldn't try something new
> here.

you could just over ride it, but ok

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > (In reply to alexander :surkov from comment #7)
> > > (In reply to Trevor Saunders (:tbsaunde) from c
> > > > >+nsAccessibilityService::UpdateLabelValue(nsIPresShell* aPresShell,
> > > > >+                                         nsIContent* aLabelElm)
> > > > >+{
> > > > >+  DocAccessible* document = GetDocAccessible(aPresShell);
> > > > can we assert there's always a doc accessible?
> > > 
> > > won't we assert for things like hidden windows?
> > 
> > well, it only gets called if the window is being layed out and the  content
> > has a frame.  So I would assert that either such windows shouldn't get layed
> > out or they should have accessibles.
> 
> per bug 794041 comment #8 hidden windows have frame tree. Do you mean that
> method isn't trigger because things aren't drawn?

I would expec that is called, but I also believe that its a bug that there is a frame tree for the hidden window. (not to mention my impression is that that sort of use of the hidden window is a hack).  In any case this seems irrelavent.

Comment 13

[Mounir Lamouri (:mounir)]

Any news regarding this bug? It is blocking a dozen of patches in my patch queue :(

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 15

[Trevor Saunders (:tbsaunde)]

Comment on attachment 719301 [details] [diff] [review]
patch3

>+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
>+  if (textBoxFrame) {
>+    mValueTextLeaf = new XULLabelTextLeafAccessible(mContent, mDoc);
>+    if (mDoc->BindToDocument(mValueTextLeaf, nullptr)) {
>+      nsAutoString text;
>+      textBoxFrame->GetCroppedTitle(text);
>+      mValueTextLeaf->SetText(text);

so I think using nsAutoString here is actually harmful, because if the string is short it might get coppied onto the stack and then into a different string buffer, or maybe the stack stoarge just isn't used I'm not sure, but either way nsString would work fine.

>+XULLabelAccessible::UpdateLabelValue(const nsString& aValue)
>+{
>+#ifdef A11Y_LOG
>+  if (logging::IsEnabled(logging::eText)) {
>+    logging::MsgBegin("TEXT", "text may be changed (xul:label @value update)");
>+    logging::Node("container", mContent);
>+    logging::MsgEntry("old text '%s'",
>+                      NS_ConvertUTF16toUTF8(mValueTextLeaf->Text()).get());
>+    logging::MsgEntry("new text: '%s'",
>+                      NS_ConvertUTF16toUTF8(aValue).get());
>+    logging::MsgEnd();
>+  }
>+#endif

follow up to have TextUpdater do this?

>   virtual bool ComputesOwnOverflowArea();
> 
>+  void GetCroppedTitle(nsString& aTitle) const { aTitle = mCroppedTitle; }

alternatively if roc's fine with it you could make this return nsDependantString

Comment 16

[alexander :surkov (:asurkov)]

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

> so I think using nsAutoString here is actually harmful, because if the
> string is short it might get coppied onto the stack and then into a
> different string buffer, or maybe the stack stoarge just isn't used I'm not
> sure, but either way nsString would work fine.

previously that was a guide to use nsAutoString for local variables. Is that something obsolette these days?

> >+XULLabelAccessible::UpdateLabelValue(const nsString& aValue)
> >+{
> >+#ifdef A11Y_LOG
> >+  if (logging::IsEnabled(logging::eText)) {
> >+    logging::MsgBegin("TEXT", "text may be changed (xul:label @value update)");
> >+    logging::Node("container", mContent);
> >+    logging::MsgEntry("old text '%s'",
> >+                      NS_ConvertUTF16toUTF8(mValueTextLeaf->Text()).get());
> >+    logging::MsgEntry("new text: '%s'",
> >+                      NS_ConvertUTF16toUTF8(aValue).get());
> >+    logging::MsgEnd();
> >+  }
> >+#endif
> 
> follow up to have TextUpdater do this?

I want to differentiate what is a cause of text change, having TextUpdater to log might do that tricky to achieve

> >   virtual bool ComputesOwnOverflowArea();
> > 
> >+  void GetCroppedTitle(nsString& aTitle) const { aTitle = mCroppedTitle; }
> 
> alternatively if roc's fine with it you could make this return
> nsDependantString

const nsDependantString would work but it's not big deal I guess. It's extra string copy which doesn't happen often.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > so I think using nsAutoString here is actually harmful, because if the
> > string is short it might get coppied onto the stack and then into a
> > different string buffer, or maybe the stack stoarge just isn't used I'm not
> > sure, but either way nsString would work fine.
> 
> previously that was a guide to use nsAutoString for local variables. Is that
> something obsolette these days?

no, I think its just this is sort of funny case.

> > >+XULLabelAccessible::UpdateLabelValue(const nsString& aValue)
> > >+{
> > >+#ifdef A11Y_LOG
> > >+  if (logging::IsEnabled(logging::eText)) {
> > >+    logging::MsgBegin("TEXT", "text may be changed (xul:label @value update)");
> > >+    logging::Node("container", mContent);
> > >+    logging::MsgEntry("old text '%s'",
> > >+                      NS_ConvertUTF16toUTF8(mValueTextLeaf->Text()).get());
> > >+    logging::MsgEntry("new text: '%s'",
> > >+                      NS_ConvertUTF16toUTF8(aValue).get());
> > >+    logging::MsgEnd();
> > >+  }
> > >+#endif
> > 
> > follow up to have TextUpdater do this?
> 
> I want to differentiate what is a cause of text change, having TextUpdater
> to log might do that tricky to achieve

ok

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> (In reply to alexander :surkov from comment #16)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > 
> > > so I think using nsAutoString here is actually harmful, because if the
> > > string is short it might get coppied onto the stack and then into a
> > > different string buffer, or maybe the stack stoarge just isn't used I'm not
> > > sure, but either way nsString would work fine.
> > 
> > previously that was a guide to use nsAutoString for local variables. Is that
> > something obsolette these days?
> 
> no, I think its just this is sort of funny case.

anyway should I switch to nsString here?

Comment 19

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5aa0d1771419

Comment 20

[[e-mail]]

(In reply to Trevor Saunders from comment #15)
> >+  nsTextBoxFrame* textBoxFrame = do_QueryFrame(mContent->GetPrimaryFrame());
> >+  if (textBoxFrame) {
> >+    mValueTextLeaf = new XULLabelTextLeafAccessible(mContent, mDoc);
> >+    if (mDoc->BindToDocument(mValueTextLeaf, nullptr)) {
> >+      nsAutoString text;
> >+      textBoxFrame->GetCroppedTitle(text);
> >+      mValueTextLeaf->SetText(text);
> 
> so I think using nsAutoString here is actually harmful, because if the
> string is short it might get coppied onto the stack and then into a
> different string buffer, or maybe the stack stoarge just isn't used I'm not
> sure, but either way nsString would work fine.

In this case the stack storage isn't used because GetCroppedTitle copies from a member nsString by using string buffer sharing.

Which string works best depends on the circumstances. Basically if you have an nsString anywhere in the chain then you'll get a shared buffer, in which case you want it as early on as possible to avoid copying between nsAutoString buffers, but if you're working entirely with nsAutoString then for short strings you'll avoid an expensive allocation.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5aa0d1771419