Closed Bug 748601 Opened 8 years ago Closed 8 years ago

nsMaiInterfaceText.cpp should check internal role not atk role

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: tbsaunde, Assigned: maxli)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 3 obsolete files)

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();
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → maxli
Attachment #619343 - Flags: review?(trev.saunders)
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
Attachment #619343 - Flags: review?(trev.saunders)
Attachment #619343 - Flags: review+
Attachment #619343 - Flags: feedback?(surkov.alexander)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #619343 - Attachment is obsolete: true
Attachment #619343 - Flags: feedback?(surkov.alexander)
Attachment #619345 - Flags: feedback?(surkov.alexander)
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
Attachment #619345 - Flags: feedback?(surkov.alexander) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #619345 - Attachment is obsolete: true
Attachment #619684 - Flags: feedback?(surkov.alexander)
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?
Attachment #619684 - Flags: feedback?(surkov.alexander) → feedback+
Attached patch Patch v4Splinter Review
Attachment #619684 - Attachment is obsolete: true
Attachment #620249 - Flags: feedback?(surkov.alexander)
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
Attachment #620249 - Flags: feedback?(surkov.alexander) → feedback+
https://hg.mozilla.org/mozilla-central/rev/298c5504def8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.