748601 - nsMaiInterfaceText.cpp should check internal role not atk role

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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]]

Attachment: Patch v1

Comment 2

[Trevor Saunders (:tbsaunde)]

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]]

Attachment: Patch v2

Comment 4

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v3

Comment 6

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v4

Comment 8

[alexander :surkov (:asurkov)]

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 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/298c5504def8

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/298c5504def8