dexpcom nsAccessible::GroupPosition

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 624410 [details] [diff] [review]
Patch (v1)

Asking surkov for initial feedback ...
Attachment #624410 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
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 ...
(Reporter)

Comment 5

5 years ago
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).
(Assignee)

Comment 6

5 years ago
Created attachment 625114 [details] [diff] [review]
Patch (v2)

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

Comment 8

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
(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
(Reporter)

Comment 12

5 years ago
(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).
(Assignee)

Comment 13

5 years ago
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?
(Reporter)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Maybe I'm asking it wrong. Doesn't this problem/situation exist currently? In either case I think I follow you.
(Reporter)

Comment 16

5 years ago
that was scenario after your patch, before your patch everything is ok
(Assignee)

Comment 17

5 years ago
Created attachment 627011 [details] [diff] [review]
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"?
(Reporter)

Comment 18

5 years ago
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
(Assignee)

Comment 19

5 years ago
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.
(Reporter)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Created attachment 627181 [details] [diff] [review]
Patch (v4)

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+
(Reporter)

Comment 23

5 years ago
(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
(Assignee)

Comment 24

5 years ago
Created attachment 628009 [details] [diff] [review]
Patch (final)

As pushed to TRY, after fixups for nsAccessible
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=53656adc5e7a
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/cf328c06ce2e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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....
(Assignee)

Comment 28

5 years ago
Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.
(Assignee)

Comment 29

5 years ago
Bug 759305 for those keeping track.
You need to log in before you can comment on or make changes to this bug.