Closed Bug 445516 Opened 16 years ago Closed 12 years ago

Support auto-generated=true text attribute on list bullets

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: aaronlev, Assigned: drexler)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file, 4 obsolete files)

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
Summary: Support static=true for list bullets → Support static=true text attribute on list bullets
here would be nice to use accessible traversing to check accessible role.
Depends on: 445677
Summary: Support static=true text attribute on list bullets → Support auto-generated=true text attribute on list bullets
No longer called static, called auto-generated.
can be fixed after bug 445677, listbullets aren't available from DOM but we navigate the DOM in the meantime.
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++]
Assignee: surkov.alexander → nobody
Attached patch patch (obsolete) — Splinter Review
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)
(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 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).
(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.
(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?
Attached patch patch (obsolete) — Splinter Review
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)
what failures are?
Attachment #618167 - Attachment is obsolete: true
Attachment #618167 - Flags: feedback?(trev.saunders)
Attachment #618167 - Attachment is obsolete: false
Attachment #618167 - Flags: feedback?(trev.saunders)
Oops sorry. Thought i hadn't qrefreshed it before requesting feedback.
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
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 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)
(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.
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.
oh, you should test text attributes on li accessible (you do ul accessible).
Attached patch WIP (obsolete) — Splinter Review
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 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)
Attached patch WIP (obsolete) — Splinter Review
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)
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
Attached patch patchSplinter Review
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)
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+
(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?
I think it makes sense
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e45f4a0da3a
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Blocks: 762469
https://hg.mozilla.org/mozilla-central/rev/4e45f4a0da3a

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