Last Comment Bug 445516 - Support auto-generated=true text attribute on list bullets
: Support auto-generated=true text attribute on list bullets
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
Depends on: 445677
Blocks: textattra11y bulleta11y 762469
  Show dependency treegraph
 
Reported: 2008-07-16 08:04 PDT by Aaron Leventhal
Modified: 2012-06-08 04:22 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.24 KB, patch)
2012-04-23 20:01 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (8.97 KB, patch)
2012-04-24 22:22 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
WIP (24.24 KB, patch)
2012-05-22 12:25 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
WIP (24.56 KB, patch)
2012-05-28 12:22 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (25.54 KB, patch)
2012-06-01 10:01 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Aaron Leventhal 2008-07-16 08:04:19 PDT
There's only one place to support "static: true" -- for list bullets.

We actually try to expose static now. For list bullet text leaves, we use
ROLE_STATICTEXT -- see nsHTMLListBulletAccessible::GetRole(). Then in
http://mxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTextAccessible.cpp#105

We currently map that to the object attribute static=true, which is wrong. The only reason we did that is that at the time I thought object attributes on text leaves would get mapped to text attributes. We need to remove that but have look for ROLE_STATICTEXT and map that into a text attribute static=true directly.

When we fix bug 346833 we should do this for :before and :after as well.

Note: we need to talk to Pete Brunet because the IA2 spec uses object attributes and sub attributes to do this. See http://www.linuxfoundation.org/en/Accessibility/IAccessible2/ObjectAttributes#List
Comment 1 alexander :surkov 2008-07-22 07:22:03 PDT
here would be nice to use accessible traversing to check accessible role.
Comment 2 Aaron Leventhal 2008-09-11 07:36:46 PDT
No longer called static, called auto-generated.
Comment 3 Aaron Leventhal 2008-09-11 07:42:32 PDT
Also add to docs:
http://developer.mozilla.org/en/Accessibility/AT-APIs#Supported_Text_Attributes
Comment 4 alexander :surkov 2009-03-04 02:05:14 PST
can be fixed after bug 445677, listbullets aren't available from DOM but we navigate the DOM in the meantime.
Comment 5 alexander :surkov 2012-04-17 23:43:10 PDT
1) remove "auto-generated" object attribute (see nsHTMLTextAccessible::GetAttributesInternal)
2) add class AutoGeneratedTextAttr : public TTextAttr<bool> into TextAttrs.h similar to other text attrs classes and implement it (refer to the logic in nsHTMLTextAccessible::GetAttributesInternal)
3) create an instance of new class in TextAttrsMgr (see TextAttrs.cpp)
Comment 6 Andrew Quartey [:drexler] 2012-04-23 20:01:13 PDT
Created attachment 617752 [details] [diff] [review]
patch
Comment 7 Trevor Saunders (:tbsaunde) 2012-04-23 20:40:07 PDT
Comment on attachment 617752 [details] [diff] [review]
patch

>+  ExposeValue(nsIPersistentProperties *aAttributes, const bool& aValue)
>+{
>+  nsAutoString value;
>+  aValue ? value.Append(NS_LITERAL_STRING("true")) : 
>+    value.Append(NS_LITERAL_STRING("false"));
>+
>+  nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::auto_generated, value);

I don't think the local var is needed

>+ 

nit, get rid of this blank line

> nsHTMLTextAccessible::GetAttributesInternal(nsIPersistentProperties *aAttributes)
> {
>-  if (NativeRole() == roles::STATICTEXT) {
>-    nsAutoString oldValueUnused;
>-    aAttributes->SetStringProperty(NS_LITERAL_CSTRING("auto-generated"),
>-                                  NS_LITERAL_STRING("true"), oldValueUnused);
>-  }
>+  TextAttrsMgr textAttrsMgr(AsHyperText());

that doesn't work since nsHTMLTextAccessible is not a nsHyperTextAccessible, I think you want Parent()->AsHyperText(), but you should check that.

>+  textAttrsMgr.GetAttributes(aAttributes);

exposing all of the text attributes as object attributes doesn't seem right.

Alex, shouldn't we test this somehow?
Comment 8 alexander :surkov 2012-04-24 01:01:45 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> Alex, shouldn't we test this somehow?

yes, in attributes/test_text

<style>
.gencontent:before {
  content: "*";
}
.gencontent:after {
  content: "*";
}
</style>

<ul>
<li class="gencontent">item</li>
</ul>

test text attributes on html:li accessible
(0, 2) should have auto-generated text attribute
(2, 6) no auto-generated text attribute
(6, 7) should have auto-generated text attribute
Comment 9 alexander :surkov 2012-04-24 01:04:44 PDT
Comment on attachment 617752 [details] [diff] [review]
patch

Review of attachment 617752 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ +108,5 @@
>  nsresult
>  nsHTMLTextAccessible::GetAttributesInternal(nsIPersistentProperties *aAttributes)
>  {
> +  TextAttrsMgr textAttrsMgr(AsHyperText());
> +  textAttrsMgr.GetAttributes(aAttributes);

no, GetAttributesInternal is used to expose object attributes (not text attributes). TextAttrsMgr is responsible to calculate text attributes (for that you should create an instance of AutoGeneratedTextAttr and add it to attrArray array, see TextAttrsMgr::GetAttributes).
Comment 10 Andrew Quartey [:drexler] 2012-04-24 08:04:52 PDT
(In reply to alexander :surkov from comment #9)
>TextAttrsMgr is responsible to calculate text attributes (for
> that you should create an instance of AutoGeneratedTextAttr and add it to
> attrArray array, see TextAttrsMgr::GetAttributes).

That was done in the patch.
Comment 11 alexander :surkov 2012-04-24 21:19:18 PDT
(In reply to Andrew Quartey [:drexler] from comment #10)
> (In reply to alexander :surkov from comment #9)
> >TextAttrsMgr is responsible to calculate text attributes (for
> > that you should create an instance of AutoGeneratedTextAttr and add it to
> > attrArray array, see TextAttrsMgr::GetAttributes).
> 
> That was done in the patch.

Yep, just remove nsHTMLTextAccessible::GetAttributesInternal part.

btw, maybe AutoGeneratedTextAttr -> AutoGenTextAttr to keep it shorter?
Comment 12 Andrew Quartey [:drexler] 2012-04-24 22:22:01 PDT
Created attachment 618167 [details] [diff] [review]
patch

Added tests for the auto-generated attribute. These currently fail in the current patch. Most likely they are incorrect but want to rule out the possibility that other modifications are incorrect first before refocusing on them.
Comment 13 alexander :surkov 2012-04-24 22:23:35 PDT
what failures are?
Comment 14 Andrew Quartey [:drexler] 2012-04-24 22:30:02 PDT
Oops sorry. Thought i hadn't qrefreshed it before requesting feedback.
Comment 15 Andrew Quartey [:drexler] 2012-04-24 22:39:10 PDT
These are the errors i get: 

INFO | runtests.py | Running tests: end.
mochitest-a11y failed:
1093 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong end offs
et for area18 at offset 0 - got 1, expected 2
1094 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | There is no ex
pected attribute 'auto-generated'  for area18 at offset 0
1095 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Can't get text
 attributes for area18
1096 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Can't get text
 attributes for area18
To rerun your failures please run 'make mochitest-plain-rerun-failures'
c:\MozRepo\src\testing\testsuite-targets.mk:148:0: command 'errors=`grep "TEST-UNEXPECTED-" mochitest-a11y.log` ; if tes
t "$errors" ; then echo "mochitest-a11y failed:"; echo "$errors"; echo "To rerun your failures please run 'make mochites
t-plain-rerun-failures'"; exit 1; else echo "mochitest-a11y passed"; fi' failed, return code 1
Comment 16 alexander :surkov 2012-04-24 23:03:05 PDT
so the problem here in
nsIContent* currElm = nsCoreUtils::GetDOMElementFor(currAcc->GetContent());
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/TextAttrs.cpp?force=1#180

You need to make TTextAttr::Equal and GetValueFor to take nsAccessible* instead of nsIContent*. It's likely you don't need nsCoreUtils::GetDOMElementFor call (you will figure that out quickly by failures presence). If you don't need it then get rid this method at all.
Comment 17 alexander :surkov 2012-04-26 00:42:51 PDT
Comment on attachment 618167 [details] [diff] [review]
patch

Review of attachment 618167 [details] [diff] [review]:
-----------------------------------------------------------------

please update the patch to comment #16
Comment 18 Andrew Quartey [:drexler] 2012-05-08 19:01:48 PDT
(In reply to alexander :surkov from comment #16)

> You need to make TTextAttr::Equal and GetValueFor to take nsAccessible*
> instead of nsIContent*. It's likely you don't need
> nsCoreUtils::GetDOMElementFor call (you will figure that out quickly by
> failures presence). If you don't need it then get rid this method at all.

This breaks the other text attribute tests.
Comment 19 Andrew Quartey [:drexler] 2012-05-09 11:13:48 PDT
From debugging i realized that at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/TextAttrs.cpp#78, role of <li> makes it an embedded object. Furthermore, the subsequent child indices (childIdx) becomes negative and thus forces the early return leading to the test failures. Changing TTextAttr::Equal and GetValueFor to take nsAccessible* only serves to break the rest of the other tests in addition to failing the ones for the auto-generated attribute.
Comment 20 alexander :surkov 2012-05-10 04:00:37 PDT
oh, you should test text attributes on li accessible (you do ul accessible).
Comment 21 Andrew Quartey [:drexler] 2012-05-22 12:25:23 PDT
Created attachment 626129 [details] [diff] [review]
WIP

Current patch state: not fully working. 

Changed TTextAttr::Equal and GetValueFor to take nsAccessible* instead of nsIContent*. Per comment #16, nsCoreUtils::GetDOMElementFor was necessary. Below is summary of the errors i am getting. On one hand, it's obvious the auto-generated text attribute isn't being exposed correctly but then again the offsets are computed correctly. I'm still looking to find out why but suggestions are most welcome.
 




1098 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Attribute 'text-position' has wrong value. Getting default text attributes for area18 - baseline should equal baseline
1099 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | There is no expected attribute 'auto-generated' . Getting default text attributes for area18


1100 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong start offset for area18 at offset 0 - 0 should equal 0
1101 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong end offset for area18 at offset 0 - 2 should equal 2
1102 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Unexpected attribute 'auto-generated' having 'true' for area18 at offset 0
1103 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Attribute 'auto-generated' has wrong value for area18 at offset 0 -
true should equal true


1111 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong start offset for area18 at offset 2 - 2 should equal 2
1112 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong end offset for area18 at offset 2 - 6 should equal 6
1120 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | There is no expected attribute 'auto-generated'  for are
a18 at offset 2


1121 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong start offset for area18 at offset 6 - 6 should equal 6
1122 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Wrong end offset for area18 at offset 6 - 7 should equal 7
1123 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Unexpected attribute 'auto-generated' having 'true' for
area18 at offset 6
1124 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Attribute 'auto-generated' has wrong value for area18 at offset 6 -
true should equal true
Comment 22 Trevor Saunders (:tbsaunde) 2012-05-22 21:55:49 PDT
Comment on attachment 626129 [details] [diff] [review]
WIP

I can look if Alex doesn't have time, but I bet he can answer your questions better and faster than me.
Comment 23 alexander :surkov 2012-05-22 23:27:29 PDT
Comment on attachment 626129 [details] [diff] [review]
WIP

Review of attachment 626129 [details] [diff] [review]:
-----------------------------------------------------------------

canceling request

::: accessible/src/base/TextAttrs.cpp
@@ +101,5 @@
>    // "font-weight" text attribute
>    FontWeightTextAttr fontWeightTextAttr(rootFrame, frame);
>  
> +  // "auto-generated" text attribute
> +	AutoGeneratedTextAttr autoGenTextAttr(mHyperTextAcc, mOffsetAcc);

nit: correct indentation

@@ +207,5 @@
>  }
>  
>  bool
>  TextAttrsMgr::LangTextAttr::
> +  GetValueFor(nsAccessible* aElm, nsString* aValue)

nit: aElm -> aAccessible, here and everywhere below

@@ +212,2 @@
>  {
> +  return GetLang(nsCoreUtils::GetDOMElementFor(aElm->GetContent()), *aValue);

you can do GetLang(aElm->GetContent()) since GetDOMElementFor return an element in case of text node but GetLang traverse the parent chain so basically it does the same.

btw, please get rid LangTextAttr::GetLang method, it's too short to keep it

@@ +247,5 @@
> +  GetValueFor(nsAccessible* aElm, nscolor* aValue)
> +{ 
> +  nsIContent* content = nsCoreUtils::GetDOMElementFor(aElm->GetContent());
> +  if (content) {
> +    nsIFrame* frame = content->GetPrimaryFrame();

I think you can do aElm->GetContent()->GetPrimaryFrame() instead, here and below

@@ +570,5 @@
> +AutoGeneratedTextAttr(nsHyperTextAccessible* aHyperTextAcc, 
> +                      nsAccessible* aAcc) :
> +	TTextAttr<bool>(!aAcc)
> +{
> +		mRootNativeValue = aHyperTextAcc->NativeRole() == roles::STATICTEXT;

root can't be statictext because static text is a leaf always, so you can do
mRootNativeValue = false;

@@ +571,5 @@
> +                      nsAccessible* aAcc) :
> +	TTextAttr<bool>(!aAcc)
> +{
> +		mRootNativeValue = aHyperTextAcc->NativeRole() == roles::STATICTEXT;
> +		mIsRootDefined = mRootNativeValue;

just mIsRootDefined = false;

@@ +572,5 @@
> +	TTextAttr<bool>(!aAcc)
> +{
> +		mRootNativeValue = aHyperTextAcc->NativeRole() == roles::STATICTEXT;
> +		mIsRootDefined = mRootNativeValue;
> +		if (aAcc) {

it can't be null

@@ +574,5 @@
> +		mRootNativeValue = aHyperTextAcc->NativeRole() == roles::STATICTEXT;
> +		mIsRootDefined = mRootNativeValue;
> +		if (aAcc) {
> +		  mNativeValue = aAcc->NativeRole() == roles::STATICTEXT;
> +		  mIsDefined = mNativeValue;

mIsDefined = mNativeValue = (aAcc->NativeRole() == roles::STATICTEXT);

this makes us to expose auto-generated:true only (no auto-generated:false) what should be fine

@@ +575,5 @@
> +		mIsRootDefined = mRootNativeValue;
> +		if (aAcc) {
> +		  mNativeValue = aAcc->NativeRole() == roles::STATICTEXT;
> +		  mIsDefined = mNativeValue;
> +		}

nit: use 2 spaces for indentation instead tab (please fix your editor settings)

you can initialize those values in initialization list like:
TTextAttr<bool>(false), mRootNativeValue(true), mIsRootDefined(false)
{
  mIsDefined = mNativeValue = (aAcc->NativeRole() == roles::STATICTEXT);
}

@@ +582,5 @@
> +bool
> +TextAttrsMgr::AutoGeneratedTextAttr::
> +GetValueFor(nsAccessible* aElm, bool* aValue)
> +{
> +  return *aValue = aElm->NativeRole() == roles::STATICTEXT;

I'd prefer to wrap comparison by ();

@@ +591,5 @@
> +ExposeValue(nsIPersistentProperties *aAttributes, const bool& aValue)
> +{
> +	nsAccUtils::SetAccAttr(aAttributes,
> +   	                     nsGkAtoms::auto_generated,
> +		                     aValue ? NS_LITERAL_STRING("true") : 

nit: whitespace in the end of line

::: accessible/src/base/TextAttrs.h
@@ +109,5 @@
>      /**
>       * Return true if the text attribute value on the given element equals with
>       * predefined attribute value.
>       */
> +    virtual bool Equal(nsAccessible* aElm) = 0;

nit: don't forget to rename aElm in this file as well

@@ +340,5 @@
> +		virtual ~AutoGeneratedTextAttr() { }
> +		 
> +		protected:
> +		
> +		//TextAttr

nit: space after //

@@ +341,5 @@
> +		 
> +		protected:
> +		
> +		//TextAttr
> +		virtual bool GetValueFor(nsAccessible* aContent, bool* aValue);

aContent -> aAccessible

@@ +342,5 @@
> +		protected:
> +		
> +		//TextAttr
> +		virtual bool GetValueFor(nsAccessible* aContent, bool* aValue);
> +		virtual void ExposeValue(nsIPersistentProperties *aAttributes,

type* name

@@ +345,5 @@
> +		virtual bool GetValueFor(nsAccessible* aContent, bool* aValue);
> +		virtual void ExposeValue(nsIPersistentProperties *aAttributes,
> +		                         const bool& aValue);
> +		 
> +	};

and to fix indentation here

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ +73,1 @@
>    }

remove GetAttributesInternal at all (we shouldn't expose object attribute since we expose text attribute)

::: accessible/tests/mochitest/attributes/test_text.html
@@ +511,5 @@
>        };
>        testTextAttrs(ID, 39, attrs, defAttrs, 39, 44);
>  
> +      //////////////////////////////////////////////////////////////////////////
> +      // area18, "auto-generation text" tests

"auto-generated" text attribute

@@ +515,5 @@
> +      // area18, "auto-generation text" tests
> +       ID = "area18";
> +       defAttrs = buildDefaultTextAttrs(ID, "12pt");
> +      defAttrs["auto-generated"] = "false";
> +      testDefaultTextAttrs(ID, defAttrs);

there's no auto-generated default text attribute (mIsRootDefined is false), that's your test failure #1

@@ +521,5 @@
> +      defAttrs["auto-generated"] = "true";
> +      testTextAttrs(ID, 0, {}, defAttrs, 0, 2);
> +
> +      defAttrs["auto-generated"] = "false";
> +      testTextAttrs(ID, 2, {}, defAttrs, 2, 6);

it shouldn't be exposed (#3 test failure)

@@ +524,5 @@
> +      defAttrs["auto-generated"] = "false";
> +      testTextAttrs(ID, 2, {}, defAttrs, 2, 6);
> +
> +      defAttrs["auto-generated"] = "true";
> +      testTextAttrs(ID, 6, {}, defAttrs, 6, 7);

I don't see why #2 and #4 can fail, please update your patch and I can do some debugging for you if you need a help.
Comment 24 Andrew Quartey [:drexler] 2012-05-28 12:22:19 PDT
Created attachment 627762 [details] [diff] [review]
WIP

Current patch (not working): auto-generated attribute still being exposed when the offsets are calculated. Debugging this is still a bit of a hit and miss. 

errors:
==========
1101 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Unexpected attribute 'auto-generated' having 'true' for area18 at offset 0

1121 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/attributes/test_text.html | Unexpected attribute 'auto-generated' having 'true' for area18 at offset 6
Comment 25 alexander :surkov 2012-06-01 09:59:10 PDT
Andrew, btw, please fix your editor settings to
1) use linux line endings (your patch contains windows ones)
2) use two spaces instead tabs
3) no whitespaces at empty lines
Comment 26 alexander :surkov 2012-06-01 10:01:31 PDT
Created attachment 629228 [details] [diff] [review]
patch

1) updated to trunk
2) style nits fixed
3) mochitests fixed
4) get back the GetAttributesInternal() removal lost in the last patch
Comment 27 Andrew Quartey [:drexler] 2012-06-01 10:19:59 PDT
Oh so it was a wrong test. Thanks Surkov :)
Comment 28 Trevor Saunders (:tbsaunde) 2012-06-07 00:05:20 PDT
Comment on attachment 629228 [details] [diff] [review]
patch

> TextAttrsMgr::LangTextAttr::
>-  GetValueFor(nsIContent* aElm, nsString* aValue)
>+  GetValueFor(Accessible* aAccessible, nsString* aValue)
> {
>-  return GetLang(aElm, *aValue);
>+  nsCoreUtils::GetLanguageFor(aAccessible->GetContent(), mRootContent, *aValue);

why don't you need to take care of text nodes with nsCoreUtils::GetDOMElementFor()?

> TextAttrsMgr::FontSizeTextAttr::
>-  GetValueFor(nsIContent* aElm, nscoord* aValue)
>+  GetValueFor(Accessible* aAccessible, nscoord* aValue)
> {
>-  nsIFrame* frame = aElm->GetPrimaryFrame();
>+  nsIContent* content = nsCoreUtils::GetDOMElementFor(aAccessible->GetContent());
>+  nsIFrame* frame = content->GetPrimaryFrame();

why don't you need to care about null element in this case?

>+TextAttrsMgr::AutoGeneratedTextAttr::
>+  ExposeValue(nsIPersistentProperties* aAttributes, const bool& aValue)
>+{
>+  nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::auto_generated,
>+                         aValue ? NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false"));

it seems like it might make a little more sense to only set an attribute in the true case, but ok
Comment 29 alexander :surkov 2012-06-07 00:14:20 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #28)

> >-  return GetLang(aElm, *aValue);
> >+  nsCoreUtils::GetLanguageFor(aAccessible->GetContent(), mRootContent, *aValue);
> 
> why don't you need to take care of text nodes with
> nsCoreUtils::GetDOMElementFor()?

because nsCoreUtils::GetLanguageFor traverses parent chain

> > TextAttrsMgr::FontSizeTextAttr::
> >-  GetValueFor(nsIContent* aElm, nscoord* aValue)
> >+  GetValueFor(Accessible* aAccessible, nscoord* aValue)
> > {
> >-  nsIFrame* frame = aElm->GetPrimaryFrame();
> >+  nsIContent* content = nsCoreUtils::GetDOMElementFor(aAccessible->GetContent());
> >+  nsIFrame* frame = content->GetPrimaryFrame();
> 
> why don't you need to care about null element in this case?

we should traverse valid tree

> >+TextAttrsMgr::AutoGeneratedTextAttr::
> >+  ExposeValue(nsIPersistentProperties* aAttributes, const bool& aValue)
> >+{
> >+  nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::auto_generated,
> >+                         aValue ? NS_LITERAL_STRING("true") : NS_LITERAL_STRING("false"));
> 
> it seems like it might make a little more sense to only set an attribute in
> the true case, but ok

actually that's what we do in the end (mIsDefined logic prevents a call with false aValue).
Comment 30 Trevor Saunders (:tbsaunde) 2012-06-07 00:35:02 PDT
> > > TextAttrsMgr::FontSizeTextAttr::
> > >-  GetValueFor(nsIContent* aElm, nscoord* aValue)
> > >+  GetValueFor(Accessible* aAccessible, nscoord* aValue)
> > > {
> > >-  nsIFrame* frame = aElm->GetPrimaryFrame();
> > >+  nsIContent* content = nsCoreUtils::GetDOMElementFor(aAccessible->GetContent());
> > >+  nsIFrame* frame = content->GetPrimaryFrame();
> > 
> > why don't you need to care about null element in this case?
> 
> we should traverse valid tree


then shouldn't we be consistant, and remove the other checks?
Comment 31 alexander :surkov 2012-06-07 00:39:58 PDT
I think it makes sense
Comment 33 Graeme McCutcheon [:graememcc] 2012-06-08 04:22:49 PDT
https://hg.mozilla.org/mozilla-central/rev/4e45f4a0da3a

(Merged by Ed Morley)

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