768461 - add Accessible::HasNumericValue() method

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

we need to know if an accessible has a vlaue to dexpcom the nsIAccessibleValue stuff.  We might also want it for firing ValueChange  events, and maybe another place or two I don't remember off hand.

There are two reasons we want to expose a value
1. aria provides one.
2. type of accessible has a value (progress meter, slider)

we could just make this method virtual, but I think we can do better.  I think we should have a eHasvalue flag and set it if the type is correct.  Then if that flag is set we can just return true.  Otherwise we can check aria the same way we do now in Accessible::QueryInterface()

Comment 1

[alexander :surkov (:asurkov)]

I can't say I'm happy with HasValue name since I always didn't like we have two value concepts (nsIAccessible::value on text entries nsIAccessibleValue on sliders and etc) but I can't think of nothing better.

Trev, it makes sense to describe steps to fix this bug and make it mentoring bug.

Comment 2

[Trevor Saunders (:tbsaunde)]

See description for general idea.

1. Add eHasNumericvalue flag to accessible type flags in accessible/src/generic/Accessible.h after eSharedNode, and shift down bit flags for accessible types accordingly.
2. set eHasNumericvalue in the constructor of XULSliderAccessible and ProgressMeterAccessible in accessible/src/xul/XULSliderAccessible.cpp and and accessible/src/generic/ProgressMeterAccessible.cpp respectively.
3. add function Accessible::HasNumericValue() to Accessible class (see accessible/src/generic/Accessible.h / cpp)
4. in Accessible::HasNumericvalue return true if eHasNumericValue bit of mFlags is set.
5. move the logic to check if nsIAccessibleValue should be exposed from Accessible::QueryInterface() to Accessible::HasNumericValue()
5. remove the just mentioned logic from Accessible::QueryInterface() and replace it with a cal to HasNumericValue()

Comment 3

[James Kitchener (:jkitch)]

I'm working on a patch for this.

For part 4 I currently have

bool HasNumericValue() const { return mFlags & eHasNumericValue); }

What I don't understand is how to combine it with part 5.

Do aIID and aInstancePtr need to be passed as arguments to HasNumericValue?  Is aInstancePtr set independent of the eHasNumericValue test?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to James Kitchener from comment #3)
> I'm working on a patch for this.
> 
> For part 4 I currently have
> 
> bool HasNumericValue() const { return mFlags & eHasNumericValue); }
> 
> What I don't understand is how to combine it with part 5.
> 
> Do aIID and aInstancePtr need to be passed as arguments to HasNumericValue? 
> Is aInstancePtr set independent of the eHasNumericValue test?

no, I mean move only the mRoleMapEntry && mRoleMapEntry->valueRule != eNoValue bit

Comment 5

[James Kitchener (:jkitch)]

Attachment: Add HasNumericValue() method

I think this does what was asked.

It hasn't been tested yet.  Which tests should I be running?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to James Kitchener from comment #5)

> I think this does what was asked.

you need to ask a mentor for review

> It hasn't been tested yet.  Which tests should I be running?

a11y tests, I do this by: cd objdir/_test/testing/mochitest; python runtests.py --a11y --autorun

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 666201 [details] [diff] [review]
Add HasNumericValue() method

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

some styling issue

::: accessible/src/generic/Accessible.cpp
@@ +138,5 @@
>      return NS_ERROR_NO_INTERFACE;
>    }
>  
>    if (aIID.Equals(NS_GET_IID(nsIAccessibleValue))) {
> +    if(HasNumericValue()) {

nit: space after 'if' (here and below)

@@ +143,5 @@
>        *aInstancePtr = static_cast<nsIAccessibleValue*>(this);
>        NS_ADDREF_THIS();
>        return NS_OK;
>      }
>    }                       

nit: please remove whitespaces

@@ +3200,5 @@
>  
>    return level;
>  }
>  
> +bool 

nit: no space in the end of line (here and in Accessible.h file)

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 666201 [details] [diff] [review]
Add HasNumericValue() method

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

it makes sense to keep Accessible::HasNumericValue() inline, otherwise it looks good. Asking Trevor for review.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 666201 [details] [diff] [review]
Add HasNumericValue() method

># HG changeset patch
># Parent 175ab8d400d8b7c1d7de7030d861e4ff901ab47b
># User James Kitchener <[e-mail]>
>Bug 768461 Add Accessible::HasNumericValue() method
>
>diff --git a/accessible/src/generic/Accessible.cpp b/accessible/src/generic/Accessible.cpp
>--- a/accessible/src/generic/Accessible.cpp
>+++ b/accessible/src/generic/Accessible.cpp
>@@ -134,17 +134,17 @@ Accessible::QueryInterface(REFNSIID aIID
>       *aInstancePtr = static_cast<nsIAccessibleSelectable*>(this);
>       NS_ADDREF_THIS();
>       return NS_OK;
>     }
>     return NS_ERROR_NO_INTERFACE;
>   }
> 
>   if (aIID.Equals(NS_GET_IID(nsIAccessibleValue))) {
>-    if (mRoleMapEntry && mRoleMapEntry->valueRule != eNoValue) {
>+    if(HasNumericValue()) {
>       *aInstancePtr = static_cast<nsIAccessibleValue*>(this);
>       NS_ADDREF_THIS();
>       return NS_OK;
>     }
>   }                       
> 
>   if (aIID.Equals(NS_GET_IID(nsIAccessibleHyperLink))) {
>     if (IsLink()) {
>@@ -3196,16 +3196,24 @@ Accessible::GetLevelInternal()
>     } else {
>       ++ level; // level is 1-index based
>     }
>   }
> 
>   return level;
> }
> 
>+bool 
>+Accessible::HasNumericValue() const
>+  if(mFlags & eHasNumericValue)
>+    return true;
>+  return mRoleMapEntry && mRoleMapEntry->valueRule != eNoValue;

nit, blank line after statement in if

r=me with that and ALex's comments addressed

Comment 10

[alexander :surkov (:asurkov)]

James, please upload new patch so we can land it.

Comment 11

[James Kitchener (:jkitch)]

My apologies for the delay.  I am currently dealing with a few type declaration issues that resulted from inlining HasNumericValue().  (I moved the code into the header file as I assume the ability to call it from outside Accessible.cpp is desirable).

I will upload a new patch as soon as possible.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to James Kitchener from comment #11)
> I moved
> the code into the header file

Accessible-inl.h

> I will upload a new patch as soon as possible.

Thank you, James.

Comment 13

[James Kitchener (:jkitch)]

Attachment: Add HasNumericValue() method updated

Thanks.  The suggested changes have been made.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 667879 [details] [diff] [review]
Add HasNumericValue() method updated

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

it seems you changed linux style line endings to windows style (you need to configure your editor properly), thus the patch cannot be applied, there's a bunch of nits still. sorry to bother you by style nits but we need that to keep code consistent. Would you mind to file another patch?

::: accessible/src/generic/Accessible-inl.h
@@ +27,5 @@
>  
>    return ARIATransformRole(mRoleMapEntry->role);
>  }
>  
> +inline bool 

nit: extra space

::: accessible/src/generic/Accessible.h
@@ +701,5 @@
>    */
>    bool IsPrimaryForNode() const { return !(mFlags & eSharedNode); }
>  
> +  /**
> +  * Return true if the accessible has a numeric value 

nit: extra space. Set dot in the end of sentence.

@@ +703,5 @@
>  
> +  /**
> +  * Return true if the accessible has a numeric value 
> +  */
> +

nit: no new line needed

Comment 15

[alexander :surkov (:asurkov)]

removing checkin-needed keyword

Comment 16

[James Kitchener (:jkitch)]

Attachment: HasNumericValue() (further corrections)

Newlines have been fixed and the other changes made.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to James Kitchener from comment #16)
> Created attachment 667890 [details] [diff] [review]
> HasNumericValue() (further corrections)
> 
> Newlines have been fixed and the other changes made.

thank you, I will land it. If you like to get another bug then you're welcome, take a look https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed;namedcmd=mentored;list_id=4569934

Comment 18

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/integration/mozilla-inbound/rev/6de63120138e

Comment 19

[alexander :surkov (:asurkov)]

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

> If you like to get another bug then you're
> welcome, take a look
> https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed;namedcmd=mentored;
> list_id=4569934

here's correct link (Josh, thanks!):
https://bugzilla.mozilla.org/query.cgi?component=Disability%20Access%20APIs&product=Core&query_format=advanced&resolution=---&status_whiteboard=[mentor%3D&status_whiteboard_type=substring&known_name=mentored

Comment 20

[alexander :surkov (:asurkov)]

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

> here's correct link:

https://bugzilla.mozilla.org/buglist.cgi?order=Importance;resolution=---;query_based_on=mentored;status_whiteboard_type=substring;query_format=advanced;status_whiteboard=[mentor%3D;component=Disability%20Access%20APIs;product=Core;known_name=mentored

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6de63120138e