Closed Bug 740766 Opened 8 years ago Closed 8 years ago

dexpcom nsAccessible::GroupPosition

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: capella)

References

Details

Attachments

(2 files, 3 obsolete files)

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?
> Sounds ok?

yeah, I think that's fine unless we wnt to rethink how we use GetPosAndSetInternal() and LevelInternal() and AccGroupInfo  while here.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Asking surkov for initial feedback ...
Attachment #624410 - Flags: feedback?(surkov.alexander)
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?
Attachment #624410 - Flags: feedback?(surkov.alexander) → feedback+
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 ...
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).
Attached patch Patch (v2) (obsolete) — Splinter Review
This addresses Alex's concerns, so now I'm bumping to Trev for review.
Attachment #624410 - Attachment is obsolete: true
Attachment #625114 - Flags: review?(trev.saunders)
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()
Attachment #625114 - Flags: review?(trev.saunders)
(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)
(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.
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.
(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
(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).
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?
(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.
Maybe I'm asking it wrong. Doesn't this problem/situation exist currently? In either case I think I follow you.
that was scenario after your patch, before your patch everything is ok
Attached patch Patch (v3) rough (obsolete) — Splinter Review
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"?
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
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.
(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.
Attached patch Patch (v4)Splinter Review
latest version, switched to struct.

TRY push included: 
https://tbpl.mozilla.org/?tree=Try&rev=7683756ce514
Attachment #625114 - Attachment is obsolete: true
Attachment #627011 - Attachment is obsolete: true
Attachment #627181 - Flags: review?(trev.saunders)
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.
Attachment #627181 - Flags: review?(trev.saunders) → review+
(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
Attached patch Patch (final)Splinter Review
As pushed to TRY, after fixups for nsAccessible
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/cf328c06ce2e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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....
Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.
Bug 759305 for those keeping track.
You need to log in before you can comment on or make changes to this bug.