Last Comment Bug 748601 - nsMaiInterfaceText.cpp should check internal role not atk role
: nsMaiInterfaceText.cpp should check internal role not atk role
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla15
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-04-24 16:29 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-04 02:27 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.95 KB, patch)
2012-04-28 15:03 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch v2 (1.95 KB, patch)
2012-04-28 16:39 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review
Patch v3 (3.50 KB, patch)
2012-04-30 14:11 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review
Patch v4 (6.35 KB, patch)
2012-05-02 04:17 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-04-24 16:29:48 PDT
see nsMaiInterfaceText.cpp lines  56 and 165.
instead of getting the atk role for the internal role and then checking against ATK_ROLE_PASSWORD_TEXT check against roles::PASSWORD_TEXt like
if (accWrap->Role() == roles::PASSWORD_TEXT)
  doSomething();
Comment 1 Max Li [:maxli] 2012-04-28 15:03:06 PDT
Created attachment 619343 [details] [diff] [review]
Patch v1
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-28 16:12:36 PDT
Comment on attachment 619343 [details] [diff] [review]
Patch v1

>-    PRUint32 atkRole = atkRoleMap[accWrap->NativeRole()];
>-    if (atkRole == ATK_ROLE_PASSWORD_TEXT) {
>+    if (accWrap->Role() == mozilla::a11y::roles::PASSWORD_TEXT) {

add using namespace mozilla::a11y a the top part of the file after headrs and just use roles::PASSWORD_TEXT here and below
Comment 3 Max Li [:maxli] 2012-04-28 16:39:07 PDT
Created attachment 619345 [details] [diff] [review]
Patch v2
Comment 4 alexander :surkov 2012-04-29 19:08:09 PDT
Comment on attachment 619345 [details] [diff] [review]
Patch v2

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

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ +52,5 @@
>  static void
>  ConvertTexttoAsterisks(nsAccessibleWrap* accWrap, nsAString& aString)
>  {
>      // convert each char to "*" when it's "password text" 
> +    if (accWrap->Role() == roles::PASSWORD_TEXT) {

please use NativeRole(), we don't want ARIA to override this behavior since it smells like finishing

while you are here, please you two spaces indentation (indent the whole block you touch)

same below
Comment 5 Max Li [:maxli] 2012-04-30 14:11:34 PDT
Created attachment 619684 [details] [diff] [review]
Patch v3
Comment 6 alexander :surkov 2012-04-30 21:39:19 PDT
Comment on attachment 619684 [details] [diff] [review]
Patch v3

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

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ -51,5 @@
>  static void
>  ConvertTexttoAsterisks(nsAccessibleWrap* accWrap, nsAString& aString)
>  {
> -    // convert each char to "*" when it's "password text" 
> -    PRUint32 atkRole = atkRoleMap[accWrap->NativeRole()];

sorry to say that but this part was changed from yesterday since bug 716644 was landed. Could you update it to trunk please? Note, you should replace AtkRoleFor() call on these NativeRole() calls the same way you do that here, and then remove AtkRoleFor() at all (fix getRoleCB to use autogenerated switch the same way as we do in msaa/nsAccessibleWrap.cpp).

@@ +144,5 @@
>  
>  static gunichar
>  getCharacterAtOffsetCB(AtkText *aText, gint aOffset)
>  {
> +  nsAccessibleWrap *accWrap = GetAccessibleWrap(ATK_OBJECT(aText));

nit: type* name

@@ +156,3 @@
>  
> +  /* PRUnichar is unsigned short in Mozilla */
> +  /* gnuichar is guint32 in glib */

nit: please use // comment style

@@ +157,5 @@
> +  /* PRUnichar is unsigned short in Mozilla */
> +  /* gnuichar is guint32 in glib */
> +  PRUnichar uniChar;
> +  nsresult rv =
> +    accText->GetCharacterAtOffset(aOffset, &uniChar);

nit: you should be able to keep it on the same line, right?
Comment 7 Max Li [:maxli] 2012-05-02 04:17:22 PDT
Created attachment 620249 [details] [diff] [review]
Patch v4
Comment 8 alexander :surkov 2012-05-02 06:50:20 PDT
Comment on attachment 620249 [details] [diff] [review]
Patch v4

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

f=me, thanks. I'll fix nits before landing

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +733,5 @@
>    if (aAtkObj->role != ATK_ROLE_INVALID)
>      return aAtkObj->role;
>  
> +  a11y::role geckoRole = accWrap->Role();
> +  PRUint32 atkRole = 0;

you can use AtkRole

@@ +735,5 @@
>  
> +  a11y::role geckoRole = accWrap->Role();
> +  PRUint32 atkRole = 0;
> +  
> +  #define ROLE(_geckoRole, stringRole, _atkRole, macRole, msaaRole, ia2Role) \

usually we don't have an indent for preprocessor directives

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ +156,5 @@
>  
> +  // PRUnichar is unsigned short in Mozilla
> +  // gnuichar is guint32 in glib
> +  PRUnichar uniChar;
> +  nsresult rv = accText->GetCharacterAtOffset(aOffset, &uniChar);

I prefer NS_FAILED here rather than in the end
Comment 10 Ed Morley [:emorley] 2012-05-04 02:27:39 PDT
https://hg.mozilla.org/mozilla-central/rev/298c5504def8

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