873439 - Implement IAccessible2_2::relationTargetsOfType

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible2__2.html

should be a plain mapping into Accessible::RelationByType

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 3

[Trevor Saunders (:tbsaunde)]

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

>+  if (!Compatibility::IsIA2Off()) {

IA2Off is just a hack for really really ancient jaws so why don't we make it only effect IAccessible2 itself?

>+    if (IID_IAccessible2 == iid) {
>+      *ppv = static_cast<IAccessible2*>(this);
>+
>+    } else if (IID_IAccessible2_2 == iid) {

it seems like it would be better to check for the newer interface first

>+  bool matched = false;
>+  RelationType relationType;

why not use the mayBe<RelationType> trick?

>+  for (uint32_t idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {

size_t ?

>+    if (wcscmp(aType, sRelationTypePairs[idx].second) == 0) {
>+      relationType = sRelationTypePairs[idx].first;
>+      matched = true;

might as well break right?

>+  nsTArray<Accessible*> targets;
>+  Accessible* target = nullptr;
>+  while ((target = rel.Next()) &&
>+         static_cast<long>(targets.Length()) <= aMaxTargets)
>+    targets.AppendElement(target);

what happens if max targets is 0?

>+  for (int32_t i = 0; i < *aNTargets; i++) {

wouldn't long make more sense than int32_t? does it matter on win64 maybe?

>+  // IAccessible2_2
>+  virtual /* [propget] */ HRESULT STDMETHODCALLTYPE get_attribute(
>+    /* [in] */ BSTR name,
>+    /* [out, retval] */ VARIANT* attribute);

any reason to not make them override?

Comment 4

[alexander :surkov (:asurkov)]

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

> >+  if (!Compatibility::IsIA2Off()) {
> 
> IA2Off is just a hack for really really ancient jaws so why don't we make it
> only effect IAccessible2 itself?

ok

> >+    if (IID_IAccessible2 == iid) {
> >+      *ppv = static_cast<IAccessible2*>(this);
> >+
> >+    } else if (IID_IAccessible2_2 == iid) {
> 
> it seems like it would be better to check for the newer interface first

hopefully this interface will picked up :)

> >+  bool matched = false;
> >+  RelationType relationType;
> 
> why not use the mayBe<RelationType> trick?

ok

> >+  for (uint32_t idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {
> 
> size_t ?

why?

> 
> >+    if (wcscmp(aType, sRelationTypePairs[idx].second) == 0) {
> >+      relationType = sRelationTypePairs[idx].first;
> >+      matched = true;
> 
> might as well break right?
> 
> >+  nsTArray<Accessible*> targets;
> >+  Accessible* target = nullptr;
> >+  while ((target = rel.Next()) &&
> >+         static_cast<long>(targets.Length()) <= aMaxTargets)
> >+    targets.AppendElement(target);
> 
> what happens if max targets is 0?

no targets returned

> >+  for (int32_t i = 0; i < *aNTargets; i++) {
> 
> wouldn't long make more sense than int32_t? does it matter on win64 maybe?

why?

> >+  // IAccessible2_2
> >+  virtual /* [propget] */ HRESULT STDMETHODCALLTYPE get_attribute(
> >+    /* [in] */ BSTR name,
> >+    /* [out, retval] */ VARIANT* attribute);
> 
> any reason to not make them override?

you mean MOZ_OVERRIDE? it's just a copy of what IDL generates, why to care?

Comment 5

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98614ea8ce7f

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98614ea8ce7f