740764 - Restrict object attributes inheritance through documents to ARIA attributes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

GetAttributesInternal was introduced in bug 375534 to not override ARIA group attributes by native group attributes. It's not a case anymore. And I don't see any single reason now to keep them separate.

Trevor?

Comment 1

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

Yeah I can imagine some simplification/reduction in merging.

Comment 2

[Trevor Saunders (:tbsaunde)]

I can't think of any problems with this off hand.  In  terms of priorities though I think I'd rather replace nsIPersistantProperties with something nice than fix this though, but we should do both.

Comment 3

[Ye Kaiqi]

Hi, I'm interested in fixing this bug. Could you explain more about what I should do to fix it? Thank you so much!

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Ye Kaiqi from comment #3)
> Hi, I'm interested in fixing this bug. Could you explain more about what I
> should do to fix it? Thank you so much!

You should move the bodies of GetAttributesInternal (http://mxr.mozilla.org/mozilla-central/ident?i=GetAttributesInternal&filter=) into the GetAttributes() call (at point where GetAttributesInternal was called).

For example we have:
nsresult
nsAccessible::GetAttributes()
{
  // code
  GetAttributesInternal();
  // code
}
nsresult
nsAccessible::GetAttributesInternal()
{
  // get attributes internal code
}
We should have:
nsresult
nsAccessible::GetAttributes()
{
  // code
  // get attributes internal code
  // code
}

Comment 5

[Ye Kaiqi]

(In reply to alexander :surkov from comment #4)
 
> You should move the bodies of GetAttributesInternal
> (http://mxr.mozilla.org/mozilla-central/
> ident?i=GetAttributesInternal&filter=) into the GetAttributes() call (at
> point where GetAttributesInternal was called).
> 
> For example we have:
> nsresult
> nsAccessible::GetAttributes()
> {
>   // code
>   GetAttributesInternal();
>   // code
> }
> nsresult
> nsAccessible::GetAttributesInternal()
> {
>   // get attributes internal code
> }
> We should have:
> nsresult
> nsAccessible::GetAttributes()
> {
>   // code
>   // get attributes internal code
>   // code
> }

Then how to replace the previous GetAttributes() function?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Ye Kaiqi from comment #5)

> Then how to replace the previous GetAttributes() function?

not sure what exactly you mean but if you talk about inheritance then you do
nsresult
ClassInheritedFrom_nsAccessible::GetAttributes()
{
  nsAccessible::GetAttributes();
  // add class specific attributes
}

Comment 7

[Ye Kaiqi]

Hi Alex,

It seems that merging the files is OK for me, but what I got stuck is that I do not know what cpp file does in firefox. Could you explain a little bit? In the cpp file, it seems that no main function is defined. Also where can I find the information about these type, such as nsresult and so on. Thank you in advance. I think it gets me more familiar with the core.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Ye Kaiqi from comment #7)
> Hi Alex,
> 
> It seems that merging the files is OK for me,

not files but methods

> but what I got stuck is that I
> do not know what cpp file does in firefox. Could you explain a little bit?

here's GetAttributesInternal (http://mxr.mozilla.org/mozilla-central/ident?i=GetAttributesInternal&filter=), you need to merge those with GetAttributes. That means if some class have GetAttributes and GetAttributesInternal() then you replace of GetAttributesInternal call in GetAttributes by implementation of GetAttributesInternal. If the class has GetAttributesInternal and doesn't GetAttributes then you rename GetAttributesInternal to getAttributes.

> In the cpp file, it seems that no main function is defined. Also where can I
> find the information about these type, such as nsresult and so on.

I think most of base methods are defined in xpcom folder. You can use http://mxr.mozilla.org/mozilla-central/ to locate where types are defined.

Comment 9

[Ye Kaiqi]

Thank you so much! I understood it now.

Comment 10

[Ye Kaiqi]

I found that OuterDocAccessible.cpp used the GetAttributesInternal() in nsAccessible.cpp, and there is a GetAttributes() is nsAccessible.cpp, so I should maintain 2 GetAttributes() in nsAccessile.cpp, right?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Ye Kaiqi from comment #10)
> I found that OuterDocAccessible.cpp used the GetAttributesInternal() in
> nsAccessible.cpp, and there is a GetAttributes() is nsAccessible.cpp, so I
> should maintain 2 GetAttributes() in nsAccessile.cpp, right?

No, you should have GetAttributes on nsAccessible and OuterDocAccessible

Comment 12

[Ye Kaiqi]

Attachment: Version 0.1

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 613504 [details] [diff] [review]
Version 0.1

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

everywhere please do: type* aName (not type *aName);

you should remove nsApplicationAccessible::GetAttributes, fix nsDocAccessible::GetAttributes, msaa/nsApplicationAccessibleWrap

please update the patch and rerequest feedback

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +1123,5 @@
>      *aState |= states::SELECTABLE | states::SELECTED;
>  }
>  
>  nsresult
> +nsARIAGridCellAccessible::GetAttributes(nsIPersistentProperties *aAttributes)

nit: type* aName

::: accessible/src/base/nsAccessible.cpp
@@ +1263,5 @@
>      // Create only if an array wasn't already passed in
>      attributes = do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);
>      NS_ENSURE_TRUE(attributes, NS_ERROR_OUT_OF_MEMORY);
>      NS_ADDREF(*aAttributes = attributes);
>    }

you should create
NS_IMETHODIMP
nsAccessible::GetAttributes(nsIPersistentProperties** aAttributes)
and put this code into it and call GetAttributes(attributes)

otherwise nsIAccessible::attributes is not implemented

::: accessible/src/base/nsAccessible.h
@@ +228,5 @@
>    /**
>     * Returns attributes for accessible without explicitly setted ARIA
>     * attributes.
>     */
> +  virtual NS_IMETHODIMP GetAttributes(nsIPersistentProperties **aAttributes);

nsresult, nsIPersistentProperties* aAttributes

Comment 14

[Ye Kaiqi]

Attachment: patch 2

Changes:
1. type* aName
2. remove nsApplicationAccessible::GetAttributes
3. change nsAccessible.cpp
4. for fixing nsDocAccessible::GetAttributes, to me, it is correct. It just calls GetAttributes in nsAccessible.cpp. The correct function is called.
5. What to do with msaa/nsApplicationAccessibleWrap

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 613599 [details] [diff] [review]
patch 2

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

approach is fine, I hope Trevor can do review

::: accessible/src/base/nsDocAccessible.cpp
@@ +353,5 @@
>  
>  }
>  
>  NS_IMETHODIMP
> +nsDocAccessible::GetAttributes(nsIPersistentProperties** aAttributes)

nsIPresistentProperties* aAttributes, i.e. use internal GetAttributes version (not XPCOM one)

@@ +1314,5 @@
>  void*
>  nsDocAccessible::GetNativeWindow() const
>  {
>    if (!mPresShell)
> +    return nsnull;Get

misspelling

Comment 16

[Ye Kaiqi]

Attachment: patch 3

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 613625 [details] [diff] [review]
patch 3

>+nsARIAGridCellAccessible::GetAttributes(nsIPersistentProperties* aAttributes)
> {
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;

not needed any more accept in nsAccessible::GetAttributes(NsIPersistantProperties**)

>+  styleInfo.MarginBottom(value);
>+  nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::marginBottom, value);
>+
>+  nsresult rv = NS_OK;
>   NS_ENSURE_SUCCESS(rv, rv);

that's a nop, just remove the NS_ENSURE_SUCCESS()

>+  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;

you shouldn't need this temporary, and it causes some extra AddRef / Releases.

>+nsDocAccessible::GetAttributes(nsIPersistentProperties* aAttributes)
> {
>   nsAccessible::GetAttributes(aAttributes);
>   if (mParent) {
>     mParent->GetAttributes(aAttributes); // Add parent attributes (override inner)
>   }
>   return NS_OK;
> }
> 
>diff --git a/accessible/src/generic/OuterDocAccessible.cpp b/accessible/src/generic/OuterDocAccessible.cpp
>--- a/accessible/src/generic/OuterDocAccessible.cpp
>+++ b/accessible/src/generic/OuterDocAccessible.cpp
>@@ -92,26 +92,26 @@ OuterDocAccessible::ChildAtPoint(PRInt32
>   NS_ENSURE_TRUE(child, nsnull);
> 
>   if (aWhichChild == eDeepestChild)
>     return child->ChildAtPoint(aX, aY, eDeepestChild);
>   return child;
> }
> 
> nsresult
>-OuterDocAccessible::GetAttributesInternal(nsIPersistentProperties* aAttributes)
>+OuterDocAccessible::GetAttributes(nsIPersistentProperties* aAttributes)
> {
>   nsAutoString tag;
>   aAttributes->GetStringProperty(NS_LITERAL_CSTRING("tag"), tag);
>   if (!tag.IsEmpty()) {
>     // We're overriding the ARIA attributes on an sub document, but we don't want to
>     // override the other attributes
>     return NS_OK;
>   }
>-  return nsAccessible::GetAttributesInternal(aAttributes);
>+  return nsAccessible::GetAttributes(aAttributes);
> }

it seems like the interaction between these two methods is jumping around because of old code that nolonger exists or something.  It seems like you override the attributes on the doc with its OuterDocAccessible if the mContent for that doc is an element, which so far as I can see right now doesn't make a whole lot of sense...

Comment 18

[alexander :surkov (:asurkov)]

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

> it seems like the interaction between these two methods is jumping around
> because of old code that nolonger exists or something.  It seems like you
> override the attributes on the doc with its OuterDocAccessible if the
> mContent for that doc is an element, which so far as I can see right now
> doesn't make a whole lot of sense...

good catch. Trevor, ideas how to workaround that?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
 
> >+  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;
> 
> you shouldn't need this temporary, and it causes some extra AddRef /
> Releases.

do_CreateInstance returns nsCOMPtr_helper so we need to do tricks to assign it to raw pointer. Perhaps it's cleaner to go with nsCOMPtr temporary.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> 
> > it seems like the interaction between these two methods is jumping around
> > because of old code that nolonger exists or something.  It seems like you
> > override the attributes on the doc with its OuterDocAccessible if the
> > mContent for that doc is an element, which so far as I can see right now
> > doesn't make a whole lot of sense...
> 
> good catch. Trevor, ideas how to workaround that?

Well, I'm not really clear on what the goal of that stuff is supposed to be.

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
>  
> > >+  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;
> > 
> > you shouldn't need this temporary, and it causes some extra AddRef /
> > Releases.
> 
> do_CreateInstance returns nsCOMPtr_helper so we need to do tricks to assign
> it to raw pointer. Perhaps it's cleaner to go with nsCOMPtr temporary.

ok, so maybe we should do this

if (!*aAttributes) {
  nsCOMPtr<nsIPersistantProperties> attrs = do_CreteInstance();
  NS_ENSURE_TRUE(attrs);
  attrs.forget(aAttributes);
}

so, keep the temporary but don't do unneeded AddRef / Release

Comment 21

[alexander :surkov (:asurkov)]

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

> > good catch. Trevor, ideas how to workaround that?
> 
> Well, I'm not really clear on what the goal of that stuff is supposed to be.

I think the purpose is inheritance of ARIA object attributes from outerdoc accessible. We could have something like AppendARIAAttributes probably.

> (In reply to alexander :surkov from comment #19)
> ok, so maybe we should do this
> 
> if (!*aAttributes) {
>   nsCOMPtr<nsIPersistantProperties> attrs = do_CreteInstance();
>   NS_ENSURE_TRUE(attrs);
>   attrs.forget(aAttributes);
> }
> 
> so, keep the temporary but don't do unneeded AddRef / Release

fine with me

Comment 22

[Ye Kaiqi]

Attachment: patch v4

If there is any mistake, please give some detailed comments to help me. Thank you so much!

Comment 23

[Trevor Saunders (:tbsaunde)]

Comment on attachment 615155 [details] [diff] [review]
patch v4

don't have much time for the next few days

Comment 24

[alexander :surkov (:asurkov)]

I think we can get rid inheritance of object attributes from iframe (outerdocaccesible) to document accessible. The inheritance was used aria live regions (http://www.w3.org/TR/2010/WD-wai-aria-implementation-20100916/#document-handling_frames). Now it's not a part of the spec. Most important thing is container-foo object attributes which are used by ATs and which are still inherited from outer document accessible. I don't see much sense to inherit aria-foo live region attributes since it doesn't sound like they are useful for ATs at all and are not in sync with container-foo attributes anyway.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 615155 [details] [diff] [review]
patch v4

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

you didn't build the patch, right? You should do make sure it's compiled before posting a patch. Thanks!

::: accessible/src/base/nsAccessible.cpp
@@ +1258,5 @@
> +  if (!IsPrimaryForNode())
> +     return NS_OK;
> +
> +  // Attributes set by this method will not be used to override attributes on a sub-document accessible
> +  // when there is a <frame>/<iframe> element that spawned the sub-document

remove this comment, not valid per comment #24

@@ +1266,5 @@
> +  // Expose class because it may have useful microformat information
> +  // Let the class from an iframe's document be exposed, don't override from <iframe class>
> +  nsAutoString _class;
> +  if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::_class, _class))
> +    nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::_class, _class);

move this after mContent->IsElement check

@@ +1325,5 @@
> +
> +  // Don't calculate CSS-based object attributes when no frame (i.e.
> +  // the accessible is unattached from the tree).
> +  if (!mContent->GetPrimaryFrame())
> +    nsresult rv = NS_OK;

something weird,

btw, I think you should move CSS based attributes into the end of the method (otherwise you don't expose things like ID)

@@ +1439,5 @@
> +  
> +  if (IsDefunct())
> +    return NS_ERROR_FAILURE;
> +
> +  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;

remove it

@@ +1442,5 @@
> +
> +  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;
> +  if (!*aAttributes) {
> +    // Create only if an array wasn't already passed in
> +    nsCOMPtr<nsIPersistantProperties> attrs = do_CreateInstance();

use attributes,
do_CreateInstance() should take an argument, see the code before you patched it

@@ +1443,5 @@
> +  nsCOMPtr<nsIPersistentProperties> attributes = *aAttributes;
> +  if (!*aAttributes) {
> +    // Create only if an array wasn't already passed in
> +    nsCOMPtr<nsIPersistantProperties> attrs = do_CreateInstance();
> +    NS_ENSURE_TRUE(attrs);

NS_ENSURE_STATE

::: accessible/src/base/nsDocAccessible.cpp
@@ +359,5 @@
>    nsAccessible::GetAttributes(aAttributes);
>    if (mParent) {
>      mParent->GetAttributes(aAttributes); // Add parent attributes (override inner)
>    }
>    return NS_OK;

you can remove implementation of this method at all per comment #24

::: accessible/src/generic/OuterDocAccessible.cpp
@@ +105,5 @@
>      // We're overriding the ARIA attributes on an sub document, but we don't want to
>      // override the other attributes
>      return NS_OK;
>    }
> +  return nsAccessible::GetAttributes(aAttributes);

you can remove the implementation per comment #24

Comment 26

[alexander :surkov (:asurkov)]

Ye, it'd be great if you find a minute to finish this patch. Please let me know if you need a help or have questions.

Comment 27

[Ye Kaiqi]

(In reply to alexander :surkov from comment #26)
> Ye, it'd be great if you find a minute to finish this patch. Please let me
> know if you need a help or have questions.

Hi Surkov,

I am doing my final exam for the past few weeks. Just finished that

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Ye Kaiqi from comment #27)
> I am doing my final exam for the past few weeks. Just finished that

cool, thank you for letting it know

Comment 29

[alexander :surkov (:asurkov)]

Ye, I'm sorry but I need to steal this bug from you since I need to get it fixed asap. Please take a look at other mentored bugs we have.

(changing the summary to not hide the behavioral change we are going to take here)

Comment 30

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 31

[Trevor Saunders (:tbsaunde)]

> (changing the summary to not hide the behavioral change we are going to take
> here)

why are we doing this? and why here?

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #31)
> > (changing the summary to not hide the behavioral change we are going to take
> > here)
> 
> why are we doing this? and why here?

this is a big cleaning up where we remove the old behavior what is not a part of the spec. I'll ask around to be on safe side.

Comment 33

[alexander :surkov (:asurkov)]

Getting the bug name back (close). David said that they targeted that issue at ARIA.next since they didn't get an agreement how to expose it. Thus I think we can change a behavior slightly here (for example, do not inherit id, class or xml-roles) to things that are reasonable.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 35

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

If behavioral changes land, my vote is to capture it in the commit message and bug summary. Note Aaron really wanted the propagation for hypothetical cases but I'm not sure it is needed.

Comment 36

[alexander :surkov (:asurkov)]

I think we end up with behavioral change since preserving the existing one doesn't make big sense (note, it's not necessary an original Aaron's idea since we might change it unintentionally during years).

A current approach of the latest patch is inherit all ARIA object attributes and nothing else. I agree the bug summary should reflect it.

Comment 37

[Trevor Saunders (:tbsaunde)]

Comment on attachment 672237 [details] [diff] [review]
patch2

>+  nsCOMPtr<nsIPersistentProperties> attributes = aAccessible->Attributes();
>+  if (attributes) {
>+    // Deal with attributes that we only need to expose in ATK.

nit, sounds obvious since we're in atk/

>+    if (aAccessible->State() & states::HASPOPUP) {
>+      // There is no ATK state for haspopup, must use object attribute to expose
>+      // the same info.

nit, move before if?

>+  if (HasNumericValue()) {
>+    // We support values, so expose the string value as well, via the valuetext
>+    // object attribute. We test for the value interface because we don't want
>+    // to expose traditional get_accValue() information such as URL's on links
>+    // and documents, or text in an input.

nit, comment before if?

>+    nsAutoString valuetext;
>+    GetValue(valuetext);

kind of funny native sumantics only method calls one that uses aria, but I don't have a better idea.

>+  GroupPos groupPos = GroupPosition();

same comment about it being funny we call a method that looks at aria.

>   /**
>    * Returns attributes for accessible without explicitly setted ARIA
>    * attributes.
>    */
>-  virtual nsresult GetAttributesInternal(nsIPersistentProperties *aAttributes);
>+  virtual already_AddRefed<nsIPersistentProperties> Attributes();

please update comment

>+ApplicationAccessible::NativeAttributes()

I wonder if we still need this, maybe its simpler to keep it though

>+HyperTextAccessible::NativeAttributes()
> {
>-  nsresult rv = AccessibleWrap::GetAttributesInternal(aAttributes);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsIPersistentProperties> attributes =
>+    AccessibleWrap::NativeAttributes();
> 
>   // Indicate when the current object uses block-level formatting
>   // via formatting: block
>   // XXX: 'formatting' attribute is deprecated and will be removed in Mozilla2,
>   // use 'display' attribute instead.

wtf is this xxx about and who is it for?  if its for AT then this is really silly because they'll never read it, and we didn't remove it, and if its for us we don't use the attribute so I don't see the problem

>   nsIFrame *frame = GetFrame();
>   if (frame && frame->GetType() == nsGkAtoms::blockFrame) {
>-    nsAutoString oldValueUnused;
>-    aAttributes->SetStringProperty(NS_LITERAL_CSTRING("formatting"), NS_LITERAL_STRING("block"),
>-                                   oldValueUnused);
>+    nsAutoString unused;
>+    attributes->SetStringProperty(NS_LITERAL_CSTRING("formatting"),
>+                                  NS_LITERAL_STRING("block"), unused);

btw what is purpose of this attribute and why doesn't it get set for things that inherit from nsBlockFrame

Comment 38

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #36)
> I think we end up with behavioral change since preserving the existing one
> doesn't make big sense (note, it's not necessary an original Aaron's idea
> since we might change it unintentionally during years).

I'm not  sure I care if it was original idea or not given that overriding some random set of attributes with ones from document container is clearly a bit crazy.

> A current approach of the latest patch is inherit all ARIA object attributes
> and nothing else. I agree the bug summary should reflect it.

I'm not sure that's a great idea, but I don't really see it hurting much.

Comment 39

[alexander :surkov (:asurkov)]

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

> I'm not  sure I care if it was original idea or not given that overriding
> some random set of attributes with ones from document container is clearly a
> bit crazy.

I think live region override is reasonable, I'm not sure about other things.

> > A current approach of the latest patch is inherit all ARIA object attributes
> > and nothing else. I agree the bug summary should reflect it.
> 
> I'm not sure that's a great idea, but I don't really see it hurting much.

I'll file a bug to sort this out.

Comment 40

[alexander :surkov (:asurkov)]

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

> >+ApplicationAccessible::NativeAttributes()
> 
> I wonder if we still need this, maybe its simpler to keep it though

yes, Accessible::NativeAttributes() assumes not null mContent, AppAccWrap has NativeAttributes

> btw what is purpose of this attribute and why doesn't it get set for things
> that inherit from nsBlockFrame

it was an analogue of today's display

Comment 41

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/33048b4879e2

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/33048b4879e2