The default bug view has changed. See this FAQ.

Support auto-generated=true text attribute on list bullets

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Aaron Leventhal, Assigned: drexler)

Tracking

(Blocks: 2 bugs, {access})

unspecified
mozilla16
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Summary: Support static=true for list bullets → Support static=true text attribute on list bullets

Comment 1

9 years ago
here would be nice to use accessible traversing to check accessible role.
Depends on: 445677
(Reporter)

Updated

9 years ago
Summary: Support static=true text attribute on list bullets → Support auto-generated=true text attribute on list bullets
(Reporter)

Comment 2

9 years ago
No longer called static, called auto-generated.
(Reporter)

Comment 3

9 years ago
Also add to docs:
http://developer.mozilla.org/en/Accessibility/AT-APIs#Supported_Text_Attributes

Comment 4

8 years ago
can be fixed after bug 445677, listbullets aren't available from DOM but we navigate the DOM in the meantime.

Updated

8 years ago
Blocks: 517453

Comment 5

5 years ago
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)
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]

Updated

5 years ago
Assignee: surkov.alexander → nobody
(Assignee)

Comment 6

5 years ago
Created attachment 617752 [details] [diff] [review]
patch
Assignee: nobody → andrew.quartey
Status: NEW → ASSIGNED
Attachment #617752 - Flags: feedback?(trev.saunders)
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?
Attachment #617752 - Flags: feedback?(trev.saunders)

Comment 8

5 years ago
(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

5 years ago
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).
(Assignee)

Comment 10

5 years ago
(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

5 years ago
(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?
(Assignee)

Comment 12

5 years ago
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.
Attachment #617752 - Attachment is obsolete: true
Attachment #618167 - Flags: feedback?(trev.saunders)

Comment 13

5 years ago
what failures are?
(Assignee)

Updated

5 years ago
Attachment #618167 - Attachment is obsolete: true
Attachment #618167 - Flags: feedback?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #618167 - Attachment is obsolete: false
Attachment #618167 - Flags: feedback?(trev.saunders)
(Assignee)

Comment 14

5 years ago
Oops sorry. Thought i hadn't qrefreshed it before requesting feedback.
(Assignee)

Comment 15

5 years ago
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

5 years ago
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.
Attachment #618167 - Flags: feedback?(trev.saunders) → feedback?(surkov.alexander)

Comment 17

5 years ago
Comment on attachment 618167 [details] [diff] [review]
patch

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

please update the patch to comment #16
Attachment #618167 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
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

5 years ago
oh, you should test text attributes on li accessible (you do ul accessible).
(Assignee)

Comment 21

5 years ago
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
Attachment #618167 - Attachment is obsolete: true
Attachment #626129 - Flags: feedback?(trev.saunders)
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.
Attachment #626129 - Flags: feedback?(trev.saunders) → feedback?(surkov.alexander)

Comment 23

5 years ago
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.
Attachment #626129 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 24

5 years ago
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
Attachment #626129 - Attachment is obsolete: true
Attachment #627762 - Flags: feedback?(surkov.alexander)

Comment 25

5 years ago
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

5 years ago
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
Attachment #627762 - Attachment is obsolete: true
Attachment #627762 - Flags: feedback?(surkov.alexander)
Attachment #629228 - Flags: review?(trev.saunders)
(Assignee)

Comment 27

5 years ago
Oh so it was a wrong test. Thanks Surkov :)
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
Attachment #629228 - Flags: review?(trev.saunders) → review+

Comment 29

5 years ago
(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).
> > > 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

5 years ago
I think it makes sense

Comment 32

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e45f4a0da3a
Flags: in-testsuite+
Target Milestone: --- → mozilla16

Updated

5 years ago
Blocks: 762469
https://hg.mozilla.org/mozilla-central/rev/4e45f4a0da3a

(Merged by Ed Morley)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.