The default bug view has changed. See this FAQ.

add Accessible::HasNumericValue() method

RESOLVED FIXED in mozilla18

Status

()

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

People

(Reporter: tbsaunde, Assigned: jkitch)

Tracking

unspecified
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=trev.saunders@gmail.com][lang=C++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

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

Updated

5 years ago
Summary: add Accessible::HasValue() method → add Accessible::HasNumericValue() method
(Reporter)

Comment 2

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

Updated

5 years ago
Whiteboard: [mentor=trev.saunders@gmail.com][lang=C++]
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

5 years ago
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

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

5 years ago
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

5 years ago
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.
Attachment #666201 - Flags: review?(trev.saunders)

Updated

5 years ago
Assignee: nobody → jkitch.bug
(Reporter)

Comment 9

5 years ago
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
Attachment #666201 - Flags: review?(trev.saunders) → review+

Comment 10

5 years ago
James, please upload new patch so we can land it.
(Assignee)

Comment 11

5 years ago
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

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

Comment 13

5 years ago
Created attachment 667879 [details] [diff] [review]
Add HasNumericValue() method updated

Thanks.  The suggested changes have been made.
Attachment #666201 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 14

5 years ago
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

5 years ago
removing checkin-needed keyword
Keywords: checkin-needed
(Assignee)

Comment 16

5 years ago
Created attachment 667890 [details] [diff] [review]
HasNumericValue() (further corrections)

Newlines have been fixed and the other changes made.
Attachment #667879 - Attachment is obsolete: true

Comment 17

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

5 years ago
landed http://hg.mozilla.org/integration/mozilla-inbound/rev/6de63120138e
Target Milestone: --- → mozilla18

Comment 19

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

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/6de63120138e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.