Last Comment Bug 396166 - Accessible <xul:label value="foo"> should implement nsIAccessibleText
: Accessible <xul:label value="foo"> should implement nsIAccessibleText
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
: 625931 (view as bug list)
Depends on: 848775
Blocks: xula11y 626315 626332 345195
  Show dependency treegraph
 
Reported: 2007-09-14 05:02 PDT by Aaron Leventhal
Modified: 2013-03-07 06:46 PST (History)
5 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (24.81 KB, patch)
2013-02-20 23:54 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (26.91 KB, patch)
2013-02-21 22:53 PST, alexander :surkov
roc: review+
Details | Diff | Splinter Review
patch3 (28.67 KB, patch)
2013-02-27 20:31 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Aaron Leventhal 2007-09-14 05:02:33 PDT
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 2007-09-14 06:53:04 PDT
the same problem must exists for xul:description@value too.
Comment 2 David Bolter [:davidb] 2009-06-16 11:54:12 PDT
Mass un-assigning bugs assigned to Aaron.
Comment 3 alexander :surkov 2011-01-16 19:12:58 PST
*** Bug 625931 has been marked as a duplicate of this bug. ***
Comment 4 alexander :surkov 2011-01-16 19:23:39 PST
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 2013-02-20 23:54:25 PST
Created attachment 716400 [details] [diff] [review]
patch

2nd approach of the bug description
Comment 6 Trevor Saunders (:tbsaunde) 2013-02-21 14:27:10 PST
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 2013-02-21 20:28:56 PST
(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) 2013-02-21 20:50:33 PST
(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 2013-02-21 21:11:33 PST
(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 2013-02-21 22:53:05 PST
Created attachment 716955 [details] [diff] [review]
patch2
Comment 11 Trevor Saunders (:tbsaunde) 2013-02-22 14:18:07 PST
> > 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) 2013-02-22 14:24:35 PST
(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) 2013-02-27 04:41:38 PST
Any news regarding this bug? It is blocking a dozen of patches in my patch queue :(
Comment 14 alexander :surkov 2013-02-27 20:31:08 PST
Created attachment 719301 [details] [diff] [review]
patch3
Comment 15 Trevor Saunders (:tbsaunde) 2013-02-27 21:43:15 PST
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 2013-02-27 21:51:05 PST
(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) 2013-02-27 22:01:48 PST
(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 2013-02-27 22:14:45 PST
(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 20 neil@parkwaycc.co.uk 2013-03-01 01:18:59 PST
(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] 2013-03-01 15:52:58 PST
https://hg.mozilla.org/mozilla-central/rev/5aa0d1771419

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