nsMaiInterfaceText.cpp should check internal role not atk role

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: tbsaunde, Assigned: maxli)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 619343 [details] [diff] [review]
Patch v1
Assignee: nobody → maxli
Attachment #619343 - Flags: review?(trev.saunders)
(Reporter)

Comment 2

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

Comment 3

5 years ago
Created attachment 619345 [details] [diff] [review]
Patch v2
Attachment #619343 - Attachment is obsolete: true
Attachment #619343 - Flags: feedback?(surkov.alexander)
Attachment #619345 - Flags: feedback?(surkov.alexander)

Comment 4

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

Comment 5

5 years ago
Created attachment 619684 [details] [diff] [review]
Patch v3
Attachment #619345 - Attachment is obsolete: true
Attachment #619684 - Flags: feedback?(surkov.alexander)

Comment 6

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

Comment 7

5 years ago
Created attachment 620249 [details] [diff] [review]
Patch v4
Attachment #619684 - Attachment is obsolete: true
Attachment #620249 - Flags: feedback?(surkov.alexander)

Comment 8

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

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/298c5504def8
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/298c5504def8
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.