Joanmarie Diggs Reporter
	Description • 7 years ago

Steps to reproduce:
1. Load data:text/html,<div role="list"><div role="listitem" aria-setsize="-1">foo</div><div role="listitem" aria-setsize="-1">bar</div></div>
2. Use Accerciser to examine the list item elements

Expected results: the elements' object attributes would contain "setsize:-1"
Actual results: the elements' object attributes would contain "setsize:2"

The ARIA spec says, "Authors must set the value of aria-setsize to an integer equal to the number of items in the set. If the total number of items is unknown, authors should set the value of aria-setsize to -1." https://rawgit.com/w3c/aria/master/aria/aria.html#aria-setsize

Calculation of the setsize can result in ATs reporting an incorrect value.

	alexander :surkov (:asurkov)
	Comment 2 • 7 years ago

Comment on attachment 8887568 [details] [diff] [review]
proposed patch

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

r=me with comment addressed, thanks!

::: accessible/base/nsCoreUtils.h
@@ +226,5 @@
> +   * @param aCanBeUnknown     [in, optional] -1 is a valid value meaning unknown
> +   * @return                  true if a valid value was found
> +   */
> +  static bool GetUIntAttr(nsIContent *aContent, nsIAtom *aAttr, int32_t* aUInt,
> +                          bool aCanBeUnknown = false);

would you mind splitting this into two GetIntAttr and GetUintAttr? It's technically incorrect when GetUintAttr may return -1, whatever it means. Also that would save us from aCanBeUnknown attribute, which makes the callers less readable, since the code reader has to have a good idea what the last boolean is about.

it is the code duplication, but maybe it's not that weird as it may be seen.

	Joanmarie Diggs Reporter
	Comment 3 • 7 years ago

(In reply to alexander :surkov from comment #2)

> would you mind splitting this into two GetIntAttr and GetUintAttr? It's
> technically incorrect when GetUintAttr may return -1, whatever it means.

For what it's worth, it did bother me that a function named GetUIntAttr could return -1. And I don't mind splitting it into two functions. But before I do, please allow me share my rationale for doing it the way I did for your consideration:

In the case of aria-setsize (and also aria-rowcount and aria-colcount), the value is an uint; not an int. -1 is not a real size or count; it's a way for authors to say, "the size/count is an uint; we just don't know how large that uint happens to be." In contrast, a function named GetIntAttr should treat any int as valid. Thus creating and using it for aria-setsize (and presumably for aria-rowcount and aria-colcount) means we need to also add sanity checking wherever this new function is called. GetUIntAttr is already doing the sanity checking we need; it just needs to recognize -1 as a special value. Beyond that, if it's essential to ensure GetUIntAttr is only used to retrieve actual uint values, shouldn't aUInt be an uint32_t and not an int32_t?

> Also that would save us from aCanBeUnknown attribute, which makes the
> callers less readable, since the code reader has to have a good idea what
> the last boolean is about.

Yeah, I struggled with the argument name. If, having read my rationale above, you still think a GetIntAttr is the right approach, problem solved. Otherwise, if you have any suggestions for a more clear argument name, please let me know.

Thanks for the review. I'll do a new patch after your feedback on the above.

	alexander :surkov (:asurkov)
	Comment 4 • 7 years ago

(In reply to Joanmarie Diggs from comment #3)
> (In reply to alexander :surkov from comment #2)
> 
> > would you mind splitting this into two GetIntAttr and GetUintAttr? It's
> > technically incorrect when GetUintAttr may return -1, whatever it means.
> 
> For what it's worth, it did bother me that a function named GetUIntAttr
> could return -1. And I don't mind splitting it into two functions. But
> before I do, please allow me share my rationale for doing it the way I did
> for your consideration:

sure, and thank you for doing this :)

> In the case of aria-setsize (and also aria-rowcount and aria-colcount), the
> value is an uint; not an int. -1 is not a real size or count; it's a way for
> authors to say, "the size/count is an uint; we just don't know how large
> that uint happens to be." In contrast, a function named GetIntAttr should
> treat any int as valid. Thus creating and using it for aria-setsize (and
> presumably for aria-rowcount and aria-colcount) means we need to also add
> sanity checking wherever this new function is called. GetUIntAttr is already
> doing the sanity checking we need; it just needs to recognize -1 as a
> special value. Beyond that, if it's essential to ensure GetUIntAttr is only
> used to retrieve actual uint values, shouldn't aUInt be an uint32_t and not
> an int32_t?

would it be nicer to have AttrValueAsInt (or any other one), and then make the caller to check the result
int32_t level = AttrValueAsInt(mContent, nsGkAtoms::aria_level);
if (level > 0) {
  groupPos.level = level;
}
etc

for error handling we could use min negative number, however we don't do any error handling now.

> > Also that would save us from aCanBeUnknown attribute, which makes the
> > callers less readable, since the code reader has to have a good idea what
> > the last boolean is about.
> 
> Yeah, I struggled with the argument name.

my concern was not the argument's name, my point was booleans in arguments are not always readable, and I prefer to use named constants:
GetTastyWatermelon(mPatch, true) vs
GetTastyWatermelon(mPatch, eSliced);

	alexander :surkov (:asurkov)
	Comment 5 • 7 years ago

keeping it on Joanie's radar ;)

	Joanmarie Diggs Reporter
	Comment 6 • 7 years ago

It's not fallen off my radar. There are just other and more pressing ARIA-related things at the present time.

	alexander :surkov (:asurkov)
	Comment 7 • 7 years ago

(In reply to Joanmarie Diggs from comment #6)
> It's not fallen off my radar. There are just other and more pressing
> ARIA-related things at the present time.

sure, I didn't wanted to be intrusive, just was making sure it's not lost.

	Sylvestre Ledru [:Sylvestre]
	Comment 8 • 6 years ago

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information