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?

Yeah I can imagine some simplification/reduction in merging.

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.

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

(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 }

(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?

(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 }

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.

(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.

Thank you so much! I understood it now.

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?

(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 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

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 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 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...

(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?

(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.

(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

(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

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

Comment on attachment 615155 [details] [diff] [review] patch v4 don't have much time for the next few days

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 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

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.

(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

(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

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)

> (changing the summary to not hide the behavioral change we are going to take > here) why are we doing this? and why here?

(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.

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.

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.

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 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

(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.

(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.

(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