742657 - move nsAccUtils::GetRoleMapEntry(nsINode *aNode) into nsARIAMap

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) add namespace aria (similar to ARIAStateMap.h) into nsARIAMap.h
2) prototype nsAccUtils::GetRoleMapEntry(nsINode *aNode) there like
nsRoleMapEntry* GetRoleMap(nsINode* aNode)
3) implement it in nsARIAMap.cpp (copy it from nsAccUtils.cpp)
4) remove nsAccUtils::GetRoleMapEntry
4) remove gWAIRoleMap[], gWAIRoleMapLength and gLandmarkRoleMap from nsARIAMap (keep them as statics in nsARIAMap.cpp)

Comment 1

[pcheng [:pcheng]]

Would like to work on it.
Just wonder what to do if I want to do incremental build after patching. Thanks.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to pcheng from comment #1)
> Would like to work on it.

assigned

> Just wonder what to do if I want to do incremental build after patching.
> Thanks.

cd yourobjdirc/accessible; make; cd ../toolkit/library; make

Comment 3

[pcheng [:pcheng]]

(In reply to alexander :surkov from comment #0)
> 1) add namespace aria (similar to ARIAStateMap.h) into nsARIAMap.h
> 2) prototype nsAccUtils::GetRoleMapEntry(nsINode *aNode) there like
> nsRoleMapEntry* GetRoleMap(nsINode* aNode)
> 3) implement it in nsARIAMap.cpp (copy it from nsAccUtils.cpp)
> 4) remove nsAccUtils::GetRoleMapEntry
> 4) remove gWAIRoleMap[], gWAIRoleMapLength and gLandmarkRoleMap from
> nsARIAMap (keep them as statics in nsARIAMap.cpp)

I had some problems:
1. the namespace added to nsARIAMap.h makes the following cold in nsAccessible.h not work:
288   virtual void SetRoleMapEntry(nsRoleMapEntry *aRoleMapEntry);
2. Similarly, wouldn't removal of nsAccUtils::GetRoleMapEntry cause problem somewhere else?

Comment 4

[alexander :surkov (:asurkov)]

please attach the patch and list errors

Comment 5

[pcheng [:pcheng]]

Attachment: compilation error report

Here listed the compilation error. Would you please take a look? I know the comment does not satisfy requirement, I will work on comments and coding style later.

Here is compilation error report.

In file included from /home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:40:0:
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.h:250:26: error: expected ‘;’ at end of member declaration
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.h:250:44: error: expected ‘)’ before ‘*’ token
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:664:1: error: cannot convert ‘nsIAtom**’ to ‘mozilla::a11y::aria::nsIAtom**’ in initialization
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:669:37: error: no ‘mozilla::a11y::aria::nsRoleMapEntry* mozilla::a11y::aria::nsARIAMap::GetRoleMap(nsINode*)’ member function declared in class ‘mozilla::a11y::aria::nsARIAMap’
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:586:17: warning: ‘gWAIRoleMapLength’ defined but not used [-Wunused-variable]
/home/pc/mozilla-central/accessible/src/base/nsARIAMap.cpp:588:23: warning: ‘gLandmarkRoleMap’ defined but not used [-Wunused-variable]

Comment 6

[pcheng [:pcheng]]

Attachment: Corrected patch. Please ignore the previous one.

Please review my patch.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 613875 [details] [diff] [review]
Corrected patch. Please ignore the previous one.

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

::: accessible/src/base/nsARIAMap.h
@@ +49,5 @@
> +class nsINode;
> +
> +namespace mozilla {
> +namespace a11y {
> +namespace aria {

perhaps I wouldn't wrap everything by aria namespace for now. I would do that GetRoleMap function only

@@ +257,5 @@
> +   * @param aNode  [in] the DOM node to get the role map entry for
> +   * @return        a pointer to the role map entry for the ARIA role, or nsnull
> +   *                if none
> +   */
> +  static nsRoleMapEntry* GetRoleMap(nsINode* aNode);

move it out from nsARIAMap, just put it under aria namespace

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1137,5 @@
>            }
>  
>  #ifdef DEBUG
>            nsRoleMapEntry *tableRoleMapEntry =
> +            nsARIAMap::GetRoleMap(tableContent);

nit: you should be able to keep it on one line

@@ +1677,5 @@
>  
>    if (tag == nsGkAtoms::a) {
>      // Only some roles truly enjoy life as nsHTMLLinkAccessibles, for details
>      // see closed bug 494807.
> +    nsRoleMapEntry *roleMapEntry = nsARIAMap::GetRoleMap(aContent);

nit: type* name and everywhere below

::: accessible/src/base/nsAccessible.cpp
@@ +113,5 @@
>  #include "mozilla/dom/Element.h"
>  
>  using namespace mozilla;
>  using namespace mozilla::a11y;
> +using namespace mozilla::a11y::aria;

don't do that, just use aria:: prefix when you calls something from aria namespace

::: accessible/src/base/nsAccessible.h
@@ +845,5 @@
>  
>    nsAutoPtr<AccGroupInfo> mGroupInfo;
>    friend class AccGroupInfo;
>  
> +  mozilla::a11y::aria::nsRoleMapEntry *mRoleMapEntry; // Non-null indicates author-supplied role; possibly state & value as well

please put comment before definition

::: accessible/src/base/nsDocAccessible.cpp
@@ +80,5 @@
>  #endif
>  
>  using namespace mozilla;
>  using namespace mozilla::a11y;
> +using namespace mozilla::a11y::aria;

don't do that and everywhere below

Comment 8

[pcheng [:pcheng]]

Attachment: Updated

Please review the updated patch.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614081 [details] [diff] [review]
Updated

this is ok, but your reward for touching some really old code is a bit more cleanup :-)

>diff -r babbc38b7f52 accessible/src/base/nsARIAMap.cpp
>--- a/accessible/src/base/nsARIAMap.cpp	Sat Apr 07 10:36:49 2012 -0700
>+++ b/accessible/src/base/nsARIAMap.cpp	Wed Apr 11 14:22:29 2012 -0400
>@@ -32,22 +32,24 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "nsCoreUtils.h"
> #include "nsARIAMap.h"
> 
> #include "Role.h"

na, add nsCoreUtils.h before Role.h

>+nsRoleMapEntry*
>+GetRoleMap(nsINode *aNode)
>+{
>+  nsIContent *content = nsCoreUtils::GetRoleContent(aNode);
>+  nsAutoString roleString;
>+  if (!content ||
>+      !content->GetAttr(kNameSpaceID_None, nsGkAtoms::role, roleString) ||
>+      roleString.IsEmpty()) {
>+    // We treat role="" as if the role attribute is absent (per aria spec:8.1.1)
>+    return nsnull;
>+  }
>+
>+  nsWhitespaceTokenizer tokenizer(roleString);
>+  while (tokenizer.hasMoreTokens()) {
>+    // Do a binary search through table for the next role in role list
>+    NS_LossyConvertUTF16toASCII role(tokenizer.nextToken());
>+    PRUint32 low = 0;
>+    PRUint32 high = gWAIRoleMapLength;

I think you could use ArrayLength() here, and get rid of the constant, that shouldn't effect perf since the function is just template magic.

>+    while (low < high) {
>+      PRUint32 index = (low + high) / 2;

rename it to idx please.

>+      PRInt32 compare = PL_strcmp(role.get(), gWAIRoleMap[index].roleString);

I think gecko tends to just use strcmp() now.

>+      if (compare == 0) {
>+        // The  role attribute maps to an entry in the role table
>+        return &gWAIRoleMap[index];
>+      }

just do

if (compare == 0)
  return gWAIRoleMap[idx};

>+      if (compare < 0) {
>+        high = index;
>+      }
>+      else {
>+        low = index + 1;
>+      }

if (compare < 0)
  high = idx;
else
  idx = low + 1;

>+/**
>+ * Get the role map entry for a given DOM node. This will use the first
>+ * ARIA role if the role attribute provides a space delimited list of roles.
>+ *
>+ * @param aNode  [in] the DOM node to get the role map entry for
>+ * @return        a pointer to the role map entry for the ARIA role, or nsnull
>+ *                if none
>+ */
>+static nsRoleMapEntry* GetRoleMap(nsINode* aNode);

loose the static, afaik it has no meaning in this context.

>+
>+

nit only one blank line.

Comment 10

[pcheng [:pcheng]]

Attachment: updated

please review my updated implementation.

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 614643 [details] [diff] [review]
updated

>diff -r babbc38b7f52 accessible/src/base/nsARIAMap.cpp
>--- a/accessible/src/base/nsARIAMap.cpp	Sat Apr 07 10:36:49 2012 -0700
>+++ b/accessible/src/base/nsARIAMap.cpp	Thu Apr 12 21:15:24 2012 -0400
>@@ -33,21 +33,21 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsARIAMap.h"
>-
>+#include "nsCoreUtils.h"
> #include "Role.h"
> #include "States.h"
>-
> #include "nsIContent.h"
>+#include "nsWhitespaceTokenizer.h"

please don't remove those blank lines.

>+static nsRoleMapEntry*
>+GetRoleMap(nsINode *aNode)

shouldn't be static

>+{
>+  nsIContent *content = nsCoreUtils::GetRoleContent(aNode);

type* name not type *name

>+  nsAutoString roleString;
>+  if (!content ||
>+      !content->GetAttr(kNameSpaceID_None, nsGkAtoms::role, roleString) ||
>+      roleString.IsEmpty()) {
>+    // We treat role="" as if the role attribute is absent (per aria spec:8.1.1)
>+    return nsnull;
>+  }
>+
>+  nsWhitespaceTokenizer tokenizer(roleString);
>+  while (tokenizer.hasMoreTokens()) {
>+    // Do a binary search through table for the next role in role list
>+    NS_LossyConvertUTF16toASCII role(tokenizer.nextToken());
>+    PRUint32 low = 0;
>+    PRUint32 high = mozilla::ArrayLength(gWAIRoleMap);

mozilla:: shouldn't be needed, add using namespace mozilla with the other using decls if you need to.

>+    while (low < high) {
>+      PRUint32 idx = (low + high) / 2;
>+      PRInt32 compare = strcmp(role.get(), gWAIRoleMap[idx].roleString);
>+      if (compare == 0) 
>+        return &gWAIRoleMap[idx];
>+      if (compare < 0)
>+        high = idx;
>+      else
>+        low = idx + 1;
>+    }
>+  }
>+
>+  // Always use some entry if there is a non-empty role string
>+  // To ensure an accessible object is created
>+  return &gLandmarkRoleMap;

nit, please rename gWAIRoleMap to sWAIRoleMap and gLandMarkRoleMap -> sLandmarkRoleMap

>     PRUint32 index = 0;
>     while (mozilla::a11y::aria::MapToState(gWAIUnivStateMap[index],
>                                            aElement, &state))
>       index++;
> 
>     return state;
>   }
>+
> };

nit, leave it as is.

Comment 12

[pcheng [:pcheng]]

Attachment: updated

Please review.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 615030 [details] [diff] [review]
updated

>+GetRoleMap(nsINode *aNode)

type* name

>+    PRUint32 high = mozilla::ArrayLength(sWAIRoleMap);

not addressed from last comments.

>+      if (compare == 0) 
>+        return &sWAIRoleMap[idx];

I think you can just do return sWAIRolemap + idx;

>     // container-live, and container-live-role attributes
>     if (live.IsEmpty()) {
>-      nsRoleMapEntry *role = GetRoleMapEntry(ancestor);
>+      nsRoleMapEntry *role = aria::GetRoleMap(ancestor);

type* name here and a couple other places.

r=me with those fixed

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 615030 [details] [diff] [review]
updated

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

I'll fix nits before landing. Thank you for fixing this bug!

::: accessible/src/base/nsARIAMap.cpp
@@ +63,5 @@
>   *  There are no nsIAccessibleRole enums for the following landmark roles:
>   *    banner, contentinfo, main, navigation, note, search, secondary, seealso, breadcrumbs
>   */
>  
> +static nsRoleMapEntry sWAIRoleMap[] = 

it's worth to rename sWAIRoleMaps, since we user RoleMap term for entry of this array

::: accessible/src/base/nsAccUtils.cpp
@@ +178,1 @@
>        if (nsAccUtils::HasDefinedARIAToken(ancestor,

type* role

::: accessible/src/base/nsAccessible.h
@@ +848,5 @@
> +  
> +  /**
> +   * Non-null indicates author-supplied role; possibly state & value as well
> +   */
> +  nsRoleMapEntry *mRoleMapEntry; 

same

Comment 15

[alexander :surkov (:asurkov)]

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/5972e57175a5

nice work! if you like to take another one please take a look here https://bugzilla.mozilla.org/buglist.cgi?emailtype1=exact;resolution=---;email1=nobody%40mozilla.org;emailassigned_to1=1;component=Disability%20Access%20APIs;product=Core;status_whiteboard=mentor;status_whiteboard_type=allwordssubstr;query_format=advanced;list_id=2863340

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5972e57175a5