Last Comment Bug 768461 - add Accessible::HasNumericValue() method
: add Accessible::HasNumericValue() method
Status: RESOLVED FIXED
[mentor=trev.saunders@gmail.com][lang...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: James Kitchener (:jkitch)
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-06-26 07:39 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-10-05 04:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add HasNumericValue() method (5.98 KB, patch)
2012-09-29 06:36 PDT, James Kitchener (:jkitch)
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Add HasNumericValue() method updated (6.23 KB, patch)
2012-10-04 03:02 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
HasNumericValue() (further corrections) (6.05 KB, patch)
2012-10-04 03:41 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-06-26 07:39:21 PDT
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 2012-09-03 17:51:57 PDT
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) 2012-09-03 21:56:18 PDT
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) 2012-09-27 06:35:53 PDT
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) 2012-09-27 10:38:52 PDT
(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) 2012-09-29 06:36:33 PDT
Created attachment 666201 [details] [diff] [review]
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 2012-10-02 03:08:44 PDT
(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 2012-10-02 03:10:39 PDT
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 2012-10-02 03:12:34 PDT
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) 2012-10-02 20:05:29 PDT
Comment on attachment 666201 [details] [diff] [review]
Add HasNumericValue() method

># HG changeset patch
># Parent 175ab8d400d8b7c1d7de7030d861e4ff901ab47b
># User James Kitchener <jkitch.bug@gmail.com>
>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 2012-10-03 04:14:24 PDT
James, please upload new patch so we can land it.
Comment 11 James Kitchener (:jkitch) 2012-10-04 00:10:42 PDT
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 2012-10-04 00:24:49 PDT
(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) 2012-10-04 03:02:18 PDT
Created attachment 667879 [details] [diff] [review]
Add HasNumericValue() method updated

Thanks.  The suggested changes have been made.
Comment 14 alexander :surkov 2012-10-04 03:09:00 PDT
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 2012-10-04 03:09:23 PDT
removing checkin-needed keyword
Comment 16 James Kitchener (:jkitch) 2012-10-04 03:41:56 PDT
Created attachment 667890 [details] [diff] [review]
HasNumericValue() (further corrections)

Newlines have been fixed and the other changes made.
Comment 17 alexander :surkov 2012-10-04 03:45:50 PDT
(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 19 alexander :surkov 2012-10-04 09:51:46 PDT
(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 21 Ed Morley [:emorley] 2012-10-05 04:03:06 PDT
https://hg.mozilla.org/mozilla-central/rev/6de63120138e

Note You need to log in before you can comment on or make changes to this bug.