740766 - dexpcom nsAccessible::GroupPosition

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

namespace mozilla {
namespace a11y {
class GroupPos {
public:
  PRUint32 Level() const { return mLevel; }
  PRUint32 PosInSet() const { return mPosInSet; }
  PRUint32 SetSize() const { return mSize; }
private:
  PRUint32 mLevel;
  PRUint32 mPosInSet;
  PRUint32 mSetSize;

friend class nsAccessible;
};
}
}

// nsAccessible.cpp
mozilla::a11y::GroupPos
nsAccessible::GroupPosition()
{
  GroupPos groupPos;
  // all code
}

NS_IMETHODIMP
nsAccessible::GroupPosition(PRInt32 *aGroupLevel,
                            PRInt32 *aSimilarItemsInGroup,
                            PRInt32 *aPositionInGroup)
{
  GroupPos groupPos = GroupPosition();
  *aGroupLevel = groupPos->Level();
  // bla bla
}

// msaa/nsAccessibleWrap.cpp
nsAccessibleWrap::get_groupPosition(long *aGroupLevel,
                                    long *aSimilarItemsInGroup,
                                    long *aPositionInGroup)
{
  GroupPos groupPos = GroupPosition();
  *aGroupLevel = groupPos->Level();
}

Sounds ok?

Comment 1

[Trevor Saunders (:tbsaunde)]

> Sounds ok?

yeah, I think that's fine unless we wnt to rethink how we use GetPosAndSetInternal() and LevelInternal() and AccGroupInfo  while here.

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

Asking surkov for initial feedback ...

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 624410 [details] [diff] [review]
Patch (v1)

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

f=me

::: accessible/src/base/nsAccessible.cpp
@@ +1508,5 @@
> +  GroupPos groupPos = GroupPosition();
> +
> +  *aGroupLevel = groupPos.Level();
> +  *aSimilarItemsInGroup = groupPos.SetSize();
> +  *aPositionInGroup = groupPos.PosInSet();

you lost NS_ENSURE_ARG_POINTERS checks

::: accessible/src/base/nsAccessible.h
@@ +84,5 @@
>   eNameOK,
>   eNameFromTooltip // Tooltip was used as a name
>  };
>  
> +class GroupPos

please comment it

@@ +91,5 @@
> +  GroupPos()
> +  {
> +    mLevel = 0;
> +    mPosInSet = 0;
> +    mSetSize = 0;

basically that's what default constructor will do but if you want to provide this one then please do
GroupPos() : mLevel(0), mPosInSet(0), mSetSize(0) { }

@@ +103,5 @@
> +  PRInt32 mLevel;
> +  PRInt32 mPosInSet;
> +  PRInt32 mSetSize;
> +
> +friend class nsAccessible;

nit: fix indentation

@@ +270,5 @@
>    /**
> +   * Returns group attributes for accessible without explicitly set ARIA
> +   * attributes.
> +   */
> +  virtual mozilla::a11y::GroupPos nsAccessible::GroupPosition();

remove nsAccessible::
at least while we have only one implementation then it doesn't make sense to keep it virtual
nit: 'Returns' -> 'Return', usually we do this way
I missed that 'without explicitly set ARIA attributes'. What happens when there are ARIA attributes?

Comment 4

[Mark Capella [:capella]]

Re: What happens when there are ARIA attributes?

ARIA Atribute values (cool link: http://www.w3.org/TR/2008/WD-wai-aria-primer-20080204/#businsessreasons) are used if available due to being explicitly marked. In all other cases we calculate the values internally.

I'm not sure I understand the question... should we cover this in our tests? I don't see anything after a brief / cursory scan ...

Comment 5

[alexander :surkov (:asurkov)]

I concerned to comment:
Returns group attributes for accessible without explicitly set ARIA attributes.

because this method returns group position taking into account ARIA. And to be correct also it doesn't return group attributes. It's enough to say something like:

Return group position.
or
Return group position (level, position in set and set size).

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

This addresses Alex's concerns, so now I'm bumping to Trev for review.

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 625114 [details] [diff] [review]
Patch (v2)

> nsAccessible::GroupPosition(PRInt32 *aGroupLevel,
>                             PRInt32 *aSimilarItemsInGroup,
>                             PRInt32 *aPositionInGroup)
> {
>   NS_ENSURE_ARG_POINTER(aGroupLevel);
>-  *aGroupLevel = 0;
>-
>   NS_ENSURE_ARG_POINTER(aSimilarItemsInGroup);
>-  *aSimilarItemsInGroup = 0;
>-
>   NS_ENSURE_ARG_POINTER(aPositionInGroup);
>-  *aPositionInGroup = 0;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;

leave the IsDefunct() check, and the 0 initialization before it.

>+class GroupPos

I'd probably just declare OO bancruptsy and call this a struct with constructors, but Alex might feel differently.

>+private:
>+  PRInt32 mLevel;
>+  PRInt32 mPosInSet;
>+  PRInt32 mSetSize;

Alex, can you think of a reason we want these to be signed?

>+  mozilla::a11y::GroupPos GroupPosition();

you should convert the impls on ApplicationAccessible and nsXULTreeItemAccessibleBase too.

Also please convert the call in GetAttributes()

Comment 8

[alexander :surkov (:asurkov)]

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

> >+class GroupPos
> 
> I'd probably just declare OO bancruptsy and call this a struct with
> constructors, but Alex might feel differently.

I think I'd left class since struct doesn't seem gives us benefits. Also class approach makes us sure GroupPos is readonly, we'd need to use const if we go with struct approach.

> >+private:
> >+  PRInt32 mLevel;
> >+  PRInt32 mPosInSet;
> >+  PRInt32 mSetSize;
> 
> Alex, can you think of a reason we want these to be signed?

nope, we follow in implementation to IA2 what means '0' value is used when group position is not applicable (it's 1-based numbers)

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > >+class GroupPos
> > 
> > I'd probably just declare OO bancruptsy and call this a struct with
> > constructors, but Alex might feel differently.
> 
> I think I'd left class since struct doesn't seem gives us benefits. Also
> class approach makes us sure GroupPos is readonly, we'd need to use const if
> we go with struct approach.

yes, I meant we'd give up trying to protect those members from mutation.  Note that any class that implements GroupPosition() will need to be a friend of this class basically, making it a struct also lets you get rid of the silly accessor methods, but whatever.

Comment 10

[Mark Capella [:capella]]

Re: comment 7 and 8 ... In the new nsAccessible::GroupPosition() method, 

nsCoreUtils::GetUIntAttr() in returns us a PRInt32
mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsCoreUtils.cpp#520

nsAccUtils::SetAccGroupAttrs() accepts PRInt32's
mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccUtils.cpp#47

I kept everything signed, not knowing how far ya'll wanted to go in the direction of making this uniform.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #10)

> I kept everything signed, not knowing how far ya'll wanted to go in the
> direction of making this uniform.

you don't need to change it here but please file a bug on this

Comment 12

[alexander :surkov (:asurkov)]

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

> yes, I meant we'd give up trying to protect those members from mutation. 
> Note that any class that implements GroupPosition() will need to be a friend
> of this class basically, making it a struct also lets you get rid of the
> silly accessor methods, but whatever.

I see but let's keep the current approach for now (I like we control the access to member of GroupPos).

Comment 13

[Mark Capella [:capella]]

I've finished a few other patches and am looking at this again.

Re: comment 7, you should convert the impls on ApplicationAccessible and nsXULTreeItemAccessibleBase too. Also please convert the call in GetAttributes() 

The call to GroupPosition() in nsAccessible.cpp is clear, less so the other two.

ApplicationAccessible::GroupPosition() basically returns zeroes. I don't understand what code change would provide benefits from what we have now.

nsXULTreeItemAccessibleBase::GroupPosition() could be broken into a separate routine returning a GroupPos (like we did in nsAccessible) but the logic is different so doesn't overlap. Again, I don't understand. 

Maybe this is a future benefits vs. immediate kinda thing?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #13)

> ApplicationAccessible::GroupPosition() basically returns zeroes. I don't
> understand what code change would provide benefits from what we have now.
> 
> nsXULTreeItemAccessibleBase::GroupPosition() could be broken into a separate
> routine returning a GroupPos (like we did in nsAccessible) but the logic is
> different so doesn't overlap. Again, I don't understand. 
> 
> Maybe this is a future benefits vs. immediate kinda thing?

It's not really future benefits, it's correctness. What happens when you call nsAccessibleWrap::get_groupPosition on XUL tree accessible? If you don't override GroupPosition on XUL tree accessible then nsAccessible::GroupPosition is called. Same for application accessible.

Comment 15

[Mark Capella [:capella]]

Maybe I'm asking it wrong. Doesn't this problem/situation exist currently? In either case I think I follow you.

Comment 16

[alexander :surkov (:asurkov)]

that was scenario after your patch, before your patch everything is ok

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v3) rough

I'm not sure this is going in the right direction so I need to have my code changes looked at. 

As-is, the build fails due to GroupPosition() in nsXULTreeAccessible.cpp not being able to access the private members mLevel, etc.

I'm guessing this is related to comment 9 "any class that implements GroupPosition() will need to be a friend of this class".

I added a friend class nsXULTreeItemAccessibleBase to nsaccessible.h GroupPos class but then started getting build errors related to ambiguous references. 

Adding explicit references to mozilla::a11y:: to clear up the ambiguous references seemed to make matters worse.

I'm sorry, but my c++ needs a lot of help here ... for example what's meant by "mutation"?

Comment 18

[alexander :surkov (:asurkov)]

you could go with struct as Trevor suggested, if so please get rid 'm' prefix from member names, similar to http://mxr.mozilla.org/mozilla-central/source/gfx/2d/BaseRect.h#53

Comment 19

[Mark Capella [:capella]]

Yes. Also, I finally figured out that if I just drop the "private" part of the GroupPosition() class, the whole thing builds and tests ok.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #19)
> Also, I finally figured out that if I just drop the "private" part of
> the GroupPosition() class, the whole thing builds and tests ok.

sure, fyi class differs struct is all class members are private by default, all struct members are public by default.

Comment 21

[Mark Capella [:capella]]

Attachment: Patch (v4)

latest version, switched to struct.

TRY push included: 
https://tbpl.mozilla.org/?tree=Try&rev=7683756ce514

Comment 22

[Trevor Saunders (:tbsaunde)]

Comment on attachment 627181 [details] [diff] [review]
Patch (v4)

>+  // If ARIA is missed and the accessible is visible then calculate group
>+  // position from hierarchy.
>+  if (State() & states::INVISIBLE)
>+    return groupPos;

btw, State() is expensive so we might want to reorder this logic so we call it less often.

>+ApplicationAccessible::GroupPosition()
> {
>-  NS_ENSURE_ARG_POINTER(aGroupLevel);
>-  *aGroupLevel = 0;
>-  NS_ENSURE_ARG_POINTER(aSimilarItemsInGroup);
>-  *aSimilarItemsInGroup = 0;
>-  NS_ENSURE_ARG_POINTER(aPositionInGroup);
>-  *aPositionInGroup = 0;
>-  return NS_OK;
>+  GroupPos groupPos;
>+
>+  return groupPos;

just return GroupPos();

>   // nsAccessible
>+  virtual mozilla::a11y::GroupPos GroupPosition();

I doubt mozilla::a11y:: is actually needed since your in that namespace already.

>+nsXULTreeItemAccessibleBase::GroupPosition()
> {
>-  NS_ENSURE_ARG_POINTER(aGroupLevel);
>-  *aGroupLevel = 0;
>-
>-  NS_ENSURE_ARG_POINTER(aSimilarItemsInGroup);
>-  *aSimilarItemsInGroup = 0;
>-
>-  NS_ENSURE_ARG_POINTER(aPositionInGroup);
>-  *aPositionInGroup = 0;
>-
>-  if (IsDefunct() || !mTreeView)
>-    return NS_ERROR_FAILURE;
>+  GroupPos groupPos;
> 
>   PRInt32 level;
>   nsresult rv = mTreeView->GetLevel(mRow, &level);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv))
>+    return groupPos;

I don't think you need ot get rid of the warning here, I think NS_ENSURE_SUCCESS(rv, groupPos); should be perfectly valid.

> 
>   PRInt32 topCount = 1;
>   for (PRInt32 index = mRow - 1; index >= 0; index--) {
>     PRInt32 lvl = -1;
>     if (NS_SUCCEEDED(mTreeView->GetLevel(index, &lvl))) {
>       if (lvl < level)
>         break;
> 
>       if (lvl == level)
>         topCount++;
>     }
>   }
> 
>   PRInt32 rowCount = 0;
>   rv = mTreeView->GetRowCount(&rowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv))
>+    return groupPos;
> 
>   PRInt32 bottomCount = 0;
>   for (PRInt32 index = mRow + 1; index < rowCount; index++) {
>     PRInt32 lvl = -1;
>     if (NS_SUCCEEDED(mTreeView->GetLevel(index, &lvl))) {
>       if (lvl < level)
>         break;
> 
>       if (lvl == level)
>         bottomCount++;
>     }
>   }
> 
>-  PRInt32 setSize = topCount + bottomCount;
>-  PRInt32 posInSet = topCount;
>+  groupPos.level = level + 1;
>+  groupPos.setSize = topCount + bottomCount;
>+  groupPos.posInSet = topCount;

nit, it'd be nice if you got rid of these local variables other than groupPos, but not really needed I guess.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> >+  if (State() & states::INVISIBLE)
> >+    return groupPos;
> 
> btw, State() is expensive so we might want to reorder this logic so we call
> it less often.

btw, most expensive part of state is visibility calculation :) and we have bug 723847 on that.

mTreeView is implemented by the author, I think it's fine to hide an error here

Comment 24

[Mark Capella [:capella]]

Attachment: Patch (final)

As pushed to TRY, after fixups for nsAccessible

Comment 25

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=53656adc5e7a

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf328c06ce2e

Comment 27

[Hubert Figuiere [:hub]]

Comment on attachment 628009 [details] [diff] [review]
Patch (final)

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

::: accessible/src/generic/Accessible.h
@@ +243,5 @@
> +   * Return group position (level, position in set and set size).
> +   */
> +  virtual mozilla::a11y::GroupPos GroupPosition();
> +
> +  /**

this line here (or below) cause a warning....

Comment 28

[Mark Capella [:capella]]

Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.

Comment 29

[Mark Capella [:capella]]

Bug 759305 for those keeping track.