673689 - introduce namespace role

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

role namespace similar to states namespace should contain accessible roles constants

Comment 1

[alexander :surkov (:asurkov)]

that could be like

namespace mozilla {
namespace a11y {
namespace roles {
  enum Role {
    PUSHBUTTON
  }
};
typedef roles::Role Role;
}

// nsAccessible.h

class nsAccessible
{
  typedef mozilla::a11y::Role Role;

  Role NativeRole();
};

// nsAccessible.cpp

using namesapce a11y::mozilla;

Role
nsAccessible::NativeRole
{
}

// callee

void func()
{
  Role role = accessible->NativeRole();
  if (role == roles::PUSHBUTTON) {
   // do something
  }
}

Comment 2

[alexander :surkov (:asurkov)]

Trevor, looks good for you?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #2)
> Trevor, looks good for you?

SURE

Comment 4

[alexander :surkov (:asurkov)]

Turn into [good first bug] for advanced contributors. It's a plain enough but requires some code investigations. The fix description:

part 1.
1) create Role.h file (accessible/src/base)
2) declare role namespace as specified in comment #1
3) copy all roles from accessible/public/nsIAccessibleRole.idl into enum Role
4) replace all nsIAccessibleRole:: instances to roles:: except XPCOM methods like nsIAccessibleRetrieval::getStringRole (for that check .idl files in accessible/public src folder). Make sure .cpp files have "using namespace mozilla::a11y".

part 2.
Find methods returning a role, replace PRUint32 on Role type (see comment #1). Candidates are NativeRole(), Role(), ARIARole() and ARIARoleInternal() of nsAccessible class.

Comment 5

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_1

Need Review.

Comment 6

[Jignesh Kakadiya [:jhk]]

Attachment: patch_2

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 580066 [details] [diff] [review]
patch_2

>diff --git a/accessible/src/atk/nsAccessibleWrap.cpp b/accessible/src/atk/nsAccessibleWrap.cpp
>--- a/accessible/src/atk/nsAccessibleWrap.cpp
>+++ b/accessible/src/atk/nsAccessibleWrap.cpp
>@@ -51,16 +51,17 @@

> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

please keep in  alphabetical order

>-    PRUint32 role = parent ? parent->Role() : 0;
>-    if (role == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
>-        role == nsIAccessibleRole::ROLE_MENUITEM ||
>-        role == nsIAccessibleRole::ROLE_RADIO_MENU_ITEM ||
>-        role == nsIAccessibleRole::ROLE_CHECK_MENU_ITEM) {
>+    Role role = parent ? parent->Role() : 0;

change 0 to role::nothing please.

>       } while ((parent = parent->Parent()) &&
>-               parent->Role() != nsIAccessibleRole::ROLE_MENUBAR);
>+               parent->Role() != roles::ROLE_MENUBAR);

any chance it goes on one line?

>+                                  // Cross Platform Roles             #

atk role,  gecko role, # maybe?

(please tell me this file was just sed -i s/nsIAccessibleRole/role/)

>diff --git a/accessible/src/base/AccGroupInfo.cpp b/accessible/src/base/AccGroupInfo.cpp
>--- a/accessible/src/base/AccGroupInfo.cpp
>+++ b/accessible/src/base/AccGroupInfo.cpp
>@@ -33,20 +33,21 @@
> #include "States.h"
>+#include "Role.h"

keep in order please

>-  if (parentRole != nsIAccessibleRole::ROLE_GROUPING ||
>-      aRole != nsIAccessibleRole::ROLE_OUTLINEITEM)
>+  if (parentRole != roles::ROLE_GROUPING ||
>+      aRole != roles::ROLE_OUTLINEITEM)

try to make one line, getting rid of role_xxx will help.

>-  static PRUint32 BaseRole(PRUint32 aRole)
>+  static PRUint32 BaseRole(Role aRole)
>   {
>-    if (aRole == nsIAccessibleRole::ROLE_CHECK_MENU_ITEM ||
>-        aRole == nsIAccessibleRole::ROLE_RADIO_MENU_ITEM)
>-      return nsIAccessibleRole::ROLE_MENUITEM;
>+    if (aRole == roles::ROLE_CHECK_MENU_ITEM ||
>+        aRole == roles::ROLE_RADIO_MENU_ITEM)
>+      return roles::ROLE_MENUITEM;

blank line please.

>diff --git a/accessible/src/base/NotificationController.cpp b/accessible/src/base/NotificationController.cpp
> #include "FocusManager.h"
> #include "TextUpdater.h"
>+#include "Role.h"

keep in order

>+  enum Role {
>+  //@see accessible/public/nsIAccessibleRole.idl for means of this flags.

they're not flags, and please copy the comments.

>+  ROLE_NOTHING = 0,

remove the ROLE_ from each of these

>+typedef roles::Role Role;
>+} // namespace ally

a11y

>+} // namespace mozill

mozilla

>+
>+#endif
>+
>diff --git a/accessible/src/base/filters.cpp b/accessible/src/base/filters.cpp
>--- a/accessible/src/base/filters.cpp
>+++ b/accessible/src/base/filters.cpp
>@@ -35,16 +35,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "filters.h"
> 
> #include "nsAccessible.h"
> #include "nsAccUtils.h"
> #include "States.h"
>+#include "Role.h"

keep them in order


note to self stoped at Filters.cpp

Comment 8

[Jignesh Kakadiya [:jhk]]

Trevor , You caught me ;).

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Jignesh kakadiya from comment #8)
> Trevor , You caught me ;).

HUH?

Comment 10

[Jignesh Kakadiya [:jhk]]

> (please tell me this file was just sed -i s/nsIAccessibleRole/role/)

sed s/nsIAccessibleRole/roles/g

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 580066 [details] [diff] [review]
patch_2

>diff --git a/accessible/src/base/filters.cpp b/accessible/src/base/filters.cpp
> #include "nsAccessible.h"
> #include "nsAccUtils.h"
> #include "States.h"
>+#include "Role.h"

please keep in order.

>+  Role role = aAccessible->Role();
>+  return role == roles::ROLE_GRID_CELL ||
>+      role == roles::ROLE_ROWHEADER ||
>+      role == roles::ROLE_COLUMNHEADER;

try to merge lines

>diff --git a/accessible/src/base/nsARIAGridAccessible.cpp b/accessible/src/base/nsARIAGridAccessible.cpp
> #include "States.h"
>+#include "Role.h"

order


>-  if (role == nsIAccessibleRole::ROLE_GRID_CELL ||
>-      role == nsIAccessibleRole::ROLE_ROWHEADER ||
>-      role == nsIAccessibleRole::ROLE_COLUMNHEADER) {
>+  if (role == roles::ROLE_GRID_CELL ||
>+      role == roles::ROLE_ROWHEADER ||
>+      role == roles::ROLE_COLUMNHEADER) {

merge lines?

>-  PRUint32 tableRole = table->Role();
>-  if (tableRole != nsIAccessibleRole::ROLE_TABLE &&
>-      tableRole != nsIAccessibleRole::ROLE_TREE_TABLE)
>+  Role tableRole = table->Role();
>+  if (tableRole != roles::ROLE_TABLE &&
>+      tableRole != roles::ROLE_TREE_TABLE)

merge?

>+    if (role == roles::ROLE_GRID_CELL ||
>+        role == roles::ROLE_ROWHEADER ||
>+        role == roles::ROLE_COLUMNHEADER)

same

>-    if (role == nsIAccessibleRole::ROLE_GRID_CELL ||
>-        role == nsIAccessibleRole::ROLE_ROWHEADER ||
>-        role == nsIAccessibleRole::ROLE_COLUMNHEADER)
>+    Role role = child->Role();
>+    if (role == roles::ROLE_GRID_CELL ||
>+        role == roles::ROLE_ROWHEADER ||
>+        role == roles::ROLE_COLUMNHEADER)

same

>+  Role tableRole = table->Role();
>+  if (tableRole != roles::ROLE_TABLE &&
>+      tableRole != roles::ROLE_TREE_TABLE)

same

>diff --git a/accessible/src/base/nsARIAMap.cpp b/accessible/src/base> #include "nsIAccessibleRole.h"
> #include "States.h"
>+#include "Role.h"

order

> /**
>  *  This list of WAI-defined roles are currently hardcoded.
>  *  Eventually we will most likely be loading an RDF resource that contains this information
>  *  Using RDF will also allow for role extensibility. See bug 280138.

Alex, are we actually at all interested in this idea?

>  *
>  *  Definition of nsRoleMapEntry and nsStateMapEntry contains comments explaining this table.
>  *
>- *  When no nsIAccessibleRole enum mapping exists for an ARIA role, the
>+ *  When no roles enum mapping exists for an ARIA role, the
>  *  role will be exposed via the object attribute "xml-roles".
>  *  In addition, in MSAA, the unmapped role will also be exposed as a BSTR string role.

this seems sort of platformy, Alex how about we update the whole comment?

>  *
>- *  There are no nsIAccessibleRole enums for the following landmark roles:
>+ *  There are no roles enums for the following landmark roles:
>  *    banner, contentinfo, main, navigation, note, search, secondary, seealso, breadcrumbs

do we actually think this makes sense, or should we do something like figure?   (I haven't dug through bugs to see)

>     "timer",
>-    nsIAccessibleRole::ROLE_NOTHING,
>+    roles::ROLE_NOTHING,

seems a little funny no? maybe call it a platform text thing with aria-live like text?

>diff --git a/accessible/src/base/nsAccUtils.cpp b/accessible/src/base/nsAccUtils.cpp
> #include "States.h"
>+#include "Role.h"

order

>-  PRUint32 role = aAccessible->Role();
>+  Role role = aAccessible->Role();
> 

get rid of the blank line

>-  if (role == nsIAccessibleRole::ROLE_ROW) {
>+  if (role == roles::ROLE_ROW) {
>     nsAccessible* parent = aAccessible->Parent();
>-    if (parent && parent->Role() == nsIAccessibleRole::ROLE_TREE_TABLE) {
>+    if (parent && parent->Role() == roles::ROLE_TREE_TABLE) {
>       // It is a row inside flatten treegrid. Group level is always 1 until it
>       // is overriden by aria-level attribute.
>       return 1;
>     }

move comment above if, and get rid of braces.

>     if (itemAcc) {
>-      PRUint32 itemRole = Role(itemAcc);
>-      if (itemRole == nsIAccessibleRole::ROLE_SEPARATOR)
>+      Role itemRole = Role(itemAcc);

Role(itemAcc) -> itemAcc->Role(), and put in the if

>-      PRUint32 itemRole = Role(itemAcc);
>-      if (itemRole == nsIAccessibleRole::ROLE_SEPARATOR)
>+      Role itemRole = Role(itemAcc);
>+      if (itemRole == roles::ROLE_SEPARATOR)

same

>-    if (Role(parent) == nsIAccessibleRole::ROLE_PANE)
>+    if (Role(parent) == roles::ROLE_PANE)

same

>-  return role == nsIAccessibleRole::ROLE_MENUITEM || 
>-    role == nsIAccessibleRole::ROLE_COMBOBOX_OPTION ||
>-    role == nsIAccessibleRole::ROLE_OPTION ||
>-    role == nsIAccessibleRole::ROLE_ENTRY ||
>-    role == nsIAccessibleRole::ROLE_FLAT_EQUATION ||
>-    role == nsIAccessibleRole::ROLE_PASSWORD_TEXT ||
>-    role == nsIAccessibleRole::ROLE_TOGGLE_BUTTON ||
>-    role == nsIAccessibleRole::ROLE_GRAPHIC ||
>-    role == nsIAccessibleRole::ROLE_SLIDER ||
>-    role == nsIAccessibleRole::ROLE_PROGRESSBAR ||
>-    role == nsIAccessibleRole::ROLE_SEPARATOR;
>+  return role == roles::ROLE_MENUITEM || 
>+    role == roles::ROLE_COMBOBOX_OPTION ||
>+    role == roles::ROLE_OPTION ||
>+    role == roles::ROLE_ENTRY ||
>+    role == roles::ROLE_FLAT_EQUATION ||
>+    role == roles::ROLE_PASSWORD_TEXT ||
>+    role == roles::ROLE_TOGGLE_BUTTON ||
>+    role == roles::ROLE_GRAPHIC ||
>+    role == roles::ROLE_SLIDER ||
>+    role == roles::ROLE_PROGRESSBAR ||
>+    role == roles::ROLE_SEPARATOR;

nsAccUtils::ROle() gets xpcom role, so maybe keep nsIAccessibleRole for now

>+      Role role = Role(cell);
>       bool isHeader = moveToLeft ?
>-        role == nsIAccessibleRole::ROLE_ROWHEADER :
>-        role == nsIAccessibleRole::ROLE_COLUMNHEADER;
>+        role == roles::ROLE_ROWHEADER :
>+        role == roles::ROLE_COLUMNHEADER;

this code is using xpcom GetRole() so I'm not sure we should use internal roles here though we will eventually when more dexpcom happens

>   static bool IsText(nsIAccessible *aAcc)
>   {
>-    PRUint32 role = Role(aAcc);
>-    return role == nsIAccessibleRole::ROLE_TEXT_LEAF ||
>-           role == nsIAccessibleRole::ROLE_STATICTEXT;
>+    Role role = Role(aAcc);

Same sort of thing, probably should leave this alone.

>   static bool IsEmbeddedObject(nsIAccessible *aAcc)
>   {
>-    PRUint32 role = Role(aAcc);
>-    return role != nsIAccessibleRole::ROLE_TEXT_LEAF &&
>-           role != nsIAccessibleRole::ROLE_WHITESPACE &&
>-           role != nsIAccessibleRole::ROLE_STATICTEXT;
>+    Role role = Role(aAcc);

same

>   nsAccessible* accessible = new nsEnumRoleAccessible(aContent, weakShell,
>-                                                      nsIAccessibleRole::ROLE_GROUPING);
>+                                                      roles::ROLE_GROUPING);

make sure to update the constructor to take a role

>+              Role role = tableAccessible->Role();
>+              if (role != roles::ROLE_TABLE &&
>+                  role != roles::ROLE_TREE_TABLE) {

merge lines

>                 // No ARIA role and not in table: override role. For example,
>                 // <table role="label"><td>content</td></table>
>                 roleMapEntry = &nsARIAMap::gEmptyRoleMap;
>               }
>             }

move comment above if and get rid of braces


>+    if (roleMapEntry && roleMapEntry->role != roles::ROLE_NOTHING &&
>+        roleMapEntry->role != roles::ROLE_LINK) {
>       nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent,
>                                                                aWeakShell);

merge lines

>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

order

> nsAccessible::GetRole(PRUint32 *aRole)
> {
>   NS_ENSURE_ARG_POINTER(aRole);
>-  *aRole = nsIAccessibleRole::ROLE_NOTHING;
>+  *aRole = roles::ROLE_NOTHING;

no, this out arg should be set to an xpcom role not an internal one so leave it the way it was.

> PRUint32
> nsAccessible::ARIARoleInternal()

return role not PRUint32

> nsAccessible::NativeRole()
> {
>   return nsCoreUtils::IsXLink(mContent) ?
>-    nsIAccessibleRole::ROLE_LINK : nsIAccessibleRole::ROLE_NOTHING;
>+    roles::ROLE_LINK : roles::ROLE_NOTHING;

see if it fits on one line.

>       if (mRoleMapEntry &&
>-          (mRoleMapEntry->role == nsIAccessibleRole::ROLE_OUTLINEITEM ||
>-           mRoleMapEntry->role == nsIAccessibleRole::ROLE_ROW)) {
>+          (mRoleMapEntry->role == roles::ROLE_OUTLINEITEM ||
>+           mRoleMapEntry->role == roles::ROLE_ROW)) {

you should be able to get rid of atleast one line here.

>         if (siblingChild &&
>-            siblingChild->Role() == nsIAccessibleRole::ROLE_LIST)
>+            siblingChild->Role() == roles::ROLE_LIST)

that can probably go on a single line

> #include "mozilla/a11y/States.h"
>+#include "mozilla/ally/Role.h"

compile error a11y not ally

>+  typedef mozilla::a11y::Role Role;

Role.h already did this you shouldn't need it again.

>   inline PRUint32 ARIARole()

return role

>   /**
>    * Returns enumerated accessible role from native markup (see constants in
>    * nsIAccessibleRole). Doesn't take into account ARIA roles.

Role.h

>diff --git a/accessible/src/base/nsApplicationAccessible.cpp b/accessible/src/base/nsApplicationAccessible.cpp
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsApplicationAccessible::NativeRole()

return role

>diff --git a/accessible/src/base/nsBaseWidgetAccessible.cpp b/accessible/src/base/nsBaseWidgetAccessible.cpp
> #include "States.h"
>+#include "Role.h"

order

>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp

> #include "AccIterator.h"
> #include "States.h"
>+#include "Role.h"

same

> PRUint32
> nsDocAccessible::NativeRole()

return type

>-    PRInt32 itemType;
>+    Role itemType;

no, that's not an accessible role we're getting

>diff --git a/accessible/src/base/nsFormControlAccessible.cpp b/accessible/src/base/nsFormControlAccessible.cpp
>  * 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 "Role.h"
> 
> // NOTE: alphabetically ordered
> #include "nsFormControlAccessible.h"
> #include "nsIDOMHTMLFormElement.h"
> #include "nsIDOMHTMLInputElement.h"
> #include "nsIDOMXULElement.h"
> #include "nsIDOMXULControlElement.h"

I don't like putting Role.h first like that, but I'm not sure exactly what I prefer, Alex?

> PRUint32
> ProgressMeterAccessible<Max>::NativeRole()

return type

> PRUint32
> nsRadioButtonAccessible::NativeRole()

same

>diff --git a/accessible/src/base/nsOuterDocAccessible.cpp b/accessible/src/base/nsOuterDocAccessible.cpp
> #include "States.h"
>+#include "Role.h"

order


> PRUint32
> nsOuterDocAccessible::NativeRole()

return type

>diff --git a/accessible/src/base/nsRootAccessible.cpp b/accessible/src/base/nsRootAccessible.cpp
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsRootAccessible::NativeRole()

return type

>     PRUint32 comboboxRole = combobox->Role();

role

>+    if (comboboxRole == roles::ROLE_COMBOBOX ||
>+        comboboxRole == roles::ROLE_AUTOCOMPLETE) {

merge lines

> PRUint32
> nsTextAccessible::NativeRole()

return type

> PRUint32
> nsHTMLCanvasAccessible::NativeRole()

same

>diff --git a/accessible/src/html/nsHTMLFormControlAccessible.cpp b/accessible/src/html/nsHTMLFormControlAccessible.cpp
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsHTMLCheckboxAccessible::NativeRole()

return type


> PRUint32
> nsHTMLButtonAccessible::NativeRole()

same

> PRUint32
> nsHTML4ButtonAccessible::NativeRole()

same

> PRUint32
> nsHTMLTextFieldAccessible::NativeRole()

same

> PRUint32
> nsHTMLFileInputAccessible::NativeRole()

same

> PRUint32
> nsHTMLGroupboxAccessible::NativeRole()

same

> PRUint32
> nsHTMLLegendAccessible::NativeRole()

same

>diff --git a/accessible/src/html/nsHTMLImageAccessible.cpp b/accessible/src/html/nsHTMLImageAccessible.cpp

> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsHTMLImageAccessible::NativeRole()

returnh type

> PRUint32
> nsHTMLImageMapAccessible::NativeRole()

same

>   if (mRoleMapEntry &&
>-      mRoleMapEntry->role != nsIAccessibleRole::ROLE_NOTHING &&
>-      mRoleMapEntry->role != nsIAccessibleRole::ROLE_LINK) {
>+      mRoleMapEntry->role != roles::ROLE_NOTHING &&
>+      mRoleMapEntry->role != roles::ROLE_LINK) {

merge lines

>diff --git a/accessible/src/html/nsHTMLLinkAccessible.cpp b/accessible/src/html/nsHTMLLinkAccessible.cpp

> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsHTMLLinkAccessible::NativeRole()

same

>diff --git a/accessible/src/html/nsHTMLSelectAccessible.cpp b/accessible/src/html/nsHTMLSelectAccessible.cpp

> #include "nsTextEquivUtils.h"
> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsHTMLSelectListAccessible::NativeRole()

same

> PRUint32
> nsHTMLSelectOptionAccessible::NativeRole()


> PRUint32
> nsHTMLSelectOptGroupAccessible::NativeRole()

> PRUint32
> nsHTMLComboboxAccessible::NativeRole()

>diff --git a/accessible/src/html/nsHTMLTableAccessible.cpp b/accessible/src/html/nsHTMLTableAccessible.cpp
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

order


> PRUint32
> nsHTMLTableCellAccessible::NativeRole()

>     if (sibling && sibling->IsElement()) {
>       if (nsCoreUtils::IsHTMLTableHeader(sibling))
>-        return nsIAccessibleRole::ROLE_COLUMNHEADER;
>-      return nsIAccessibleRole::ROLE_ROWHEADER;
>+        return roles::ROLE_COLUMNHEADER;
>+      return roles::ROLE_ROWHEADER;
>     }

maybe

if (sibling && blah)
  return nsCoreUtils::blah() ? roles::foo : roles::bar;

>     if (sibling && sibling->IsElement()) {
>       if (nsCoreUtils::IsHTMLTableHeader(sibling))
>-        return nsIAccessibleRole::ROLE_COLUMNHEADER;
>-      return nsIAccessibleRole::ROLE_ROWHEADER;
>+        return roles::ROLE_COLUMNHEADER;
>+      return roles::ROLE_ROWHEADER;
>     }

same

> PRUint32
> nsHTMLTableAccessible::NativeRole()

>-  bool hasNonTableRole = (Role() != nsIAccessibleRole::ROLE_TABLE);
>+  bool hasNonTableRole = (Role() != roles::ROLE_TABLE);
>   if (hasNonTableRole) {
>     RETURN_LAYOUT_ANSWER(false, "Has role attribute");
>   }

if (Role() 1= blah)
  return_layout_answer(blah)

> PRUint32
> nsHTMLCaptionAccessible::NativeRole()

>diff --git a/accessible/src/html/nsHTMLTextAccessible.cpp b/accessible/src/html/nsHTMLTextAccessible.cpp

> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"

> PRUint32
> nsHTMLTextAccessible::NativeRole()

>   if (frame && frame->IsGeneratedContentFrame()) {
>-    return nsIAccessibleRole::ROLE_STATICTEXT;
>+    return roles::ROLE_STATICTEXT;
>   }

get rid of the braces while your here

> PRUint32
> nsHTMLHRAccessible::NativeRole()

> PRUint32
> nsHTMLBRAccessible::NativeRole()

> PRUint32
> nsHTMLLabelAccessible::NativeRole()

> PRUint32
> nsHTMLOutputAccessible::NativeRole()

> PRUint32
> nsHTMLLIAccessible::NativeRole()

> PRUint32
> nsHTMLListBulletAccessible::NativeRole()

> PRUint32
> nsHTMLListAccessible::NativeRole()

>diff --git a/accessible/src/html/nsHyperTextAccessible.cpp b/accessible/src/html/nsHyperTextAccessible.cpp
> #include "States.h"
>+#include "Role.h"

order

>+      (mRoleMapEntry->role == roles::ROLE_GRAPHIC ||
>+       mRoleMapEntry->role == roles::ROLE_IMAGE_MAP ||
>+       mRoleMapEntry->role == roles::ROLE_SLIDER ||
>+       mRoleMapEntry->role == roles::ROLE_PROGRESSBAR ||
>+       mRoleMapEntry->role == roles::ROLE_SEPARATOR)) {

merge

> PRUint32
> nsHyperTextAccessible::NativeRole()

>-    if (finalAccessible->Role() == nsIAccessibleRole::ROLE_WHITESPACE) {  // Landed on <br> hard line break
>+    if (finalAccessible->Role() == roles::ROLE_WHITESPACE) {  // Landed on <br> hard line break
>       // if aNeedsStart, set end of line exactly 1 character past line break
>       // XXX It would be cleaner if we did not have to have the hard line break check,
>       // and just got the correct results from PeekOffset() for the <br> case -- the returned offset should
>       // come after the new line, as it does in other cases.
>       ++ hyperTextOffset;  // Get past hard line break
>     }

comment before if, no braces

>     // Check for OS X "role skew"; the role constants in nsIAccessible.idl need to match the ones
>     // in nsRoleMap.h.

update the comment for the right file

>diff --git a/accessible/src/msaa/nsAccessibleWrap.cpp b/accessible/src/msaa/nsAccessibleWrap.cpp

> #include "States.h"
>+#include "Role.h"

>-  PRUint32 xpRole = xpAccessible->Role();
>+  Role xpRole = xpAccessible->Role();
>   PRUint32 msaaRole = gWindowsRoleMap[xpRole].msaaRole;

Alex, any problem with plain role?

>-  NS_ASSERTION(gWindowsRoleMap[nsIAccessibleRole::ROLE_LAST_ENTRY].msaaRole == ROLE_WINDOWS_LAST_ENTRY,
>+  NS_ASSERTION(gWindowsRoleMap[roles::ROLE_LAST_ENTRY].msaaRole == ROLE_WINDOWS_LAST_ENTRY,
>                "MSAA role map skewed");

we should really  convert all these  to build time asserts

>diff --git a/accessible/src/msaa/nsDocAccessibleWrap.cpp b/accessible/src/msaa/nsDocAccessibleWrap.cpp
--- a/accessible/src/msaa/nsDocAccessibleWrap.cpp
>+++ b/accessible/src/msaa/nsDocAccessibleWrap.cpp
>@@ -39,16 +39,17 @@
> #include "mozilla/dom/TabChild.h"
> 
> #include "nsDocAccessibleWrap.h"
> #include "ISimpleDOMDocument_i.c"
> #include "nsIAccessibilityService.h"
> #include "nsRootAccessible.h"
> #include "nsWinUtils.h"c
> #include "Statistics.h"
>+#include "Role.h"

>+  if (role != roles::ROLE_DOCUMENT &&
>+      role != roles::ROLE_APPLICATION &&
>+      role != roles::ROLE_DIALOG &&
>+      role != roles::ROLE_ALERT)

merge

>diff --git a/accessible/src/msaa/nsHTMLWin32ObjectAccessible.cpp b/accessible/src/msaa/nsHTMLWin32ObjectAccessible.cpp

> #include "States.h"
>+#include "Role.h"

order

> PRUint32
> nsHTMLWin32ObjectOwnerAccessible::NativeRole()


>diff --git a/accessible/src/xforms/nsXFormsAccessible.cpp b/accessible/src/xforms/nsXFormsAccessible.cpp

> #include "States.h"
>+#include "Role.h"


> PRUint32
> nsXFormsContainerAccessible::NativeRole()

>diff --git a/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp b/accessible/src/xforms/nsXFormsFormControlsAccessible.cpp

> #include "States.h"
>+#include "Role.h"

> PRUint32
> nsXFormsLabelAccessible::NativeRole()

> PRUint32
> nsXFormsOutputAccessible::NativeRole()

> PRUint32
> nsXFormsTriggerAccessible::NativeRole()

> PRUint32
> nsXFormsInputAccessible::NativeRole()

> PRUint32
> nsXFormsInputBooleanAccessible::NativeRole()

> PRUint32
> nsXFormsInputDateAccessible::NativeRole()

> PRUint32
> nsXFormsSecretAccessible::NativeRole()

> PRUint32
> nsXFormsRangeAccessible::NativeRole()

> PRUint32
> nsXFormsChoicesAccessible::NativeRole()

> PRUint32
> nsXFormsSelectFullAccessible::NativeRole()

> PRUint32
> nsXFormsItemCheckgroupAccessible::NativeRole()

> PRUint32
> nsXFormsItemRadiogroupAccessible::NativeRole()

> PRUint32
> nsXFormsSelectComboboxAccessible::NativeRole()

> PRUint32
> nsXFormsItemComboboxAccessible::NativeRole()

>diff --git a/accessible/src/xforms/nsXFormsWidgetsAccessible.cpp b/accessible/src/xforms/nsXFormsWidgetsAccessible.cpp

> #include "States.h"
>+#include "Role.h"


> nsXULComboboxAccessible::NativeRole()
> {
>   if (IsAutoComplete())
>-    return nsIAccessibleRole::ROLE_AUTOCOMPLETE;
>-  return nsIAccessibleRole::ROLE_COMBOBOX;
>+    return roles::ROLE_AUTOCOMPLETE;
>+  return roles::ROLE_COMBOBOX;

return IsAutoComplete() ? roles::foo : roles::bar;

>   if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>                             nsGkAtoms::password, eIgnoreCase))
>-    return nsIAccessibleRole::ROLE_PASSWORD_TEXT;
>-  return nsIAccessibleRole::ROLE_ENTRY;
>+    return roles::ROLE_PASSWORD_TEXT;
>+  return roles::ROLE_ENTRY;

blank line after first return

>   if (IsMulticolumn())
>-    return nsIAccessibleRole::ROLE_TABLE;
>-  return nsIAccessibleRole::ROLE_LISTBOX;
>+    return roles::ROLE_TABLE;
>+  return roles::ROLE_LISTBOX;

return IsMultiColumn() ? roles::foo : roles::bar;

>     PRUint32 role = cell->Role();

role

>+  bool isComboboxOption = (Role() == roles::ROLE_COMBOBOX_OPTION);
>   if (isComboboxOption) {

it appears you don't need the local variable

>-      return nsIAccessibleRole::ROLE_COMBOBOX_LIST;
>+    Role role = mParent->Role();
>+    if (role == roles::ROLE_COMBOBOX ||
>+        role == roles::ROLE_AUTOCOMPLETE) {
>+      return roles::ROLE_COMBOBOX_LIST;
>     }

merge lines get rid of braces

> 
>-    if (role == nsIAccessibleRole::ROLE_PUSHBUTTON) {
>+    if (role == roles::ROLE_PUSHBUTTON) {
>       // Some widgets like the search bar have several popups, owned by buttons.
>       nsAccessible* grandParent = mParent->Parent();
>       if (grandParent &&
>-          grandParent->Role() == nsIAccessibleRole::ROLE_AUTOCOMPLETE)
>-        return nsIAccessibleRole::ROLE_COMBOBOX_LIST;
>+          grandParent->Role() == roles::ROLE_AUTOCOMPLETE)
>+        return roles::ROLE_COMBOBOX_LIST;
>     }

same

>   return primaryCol ?
>-    static_cast<PRUint32>(nsIAccessibleRole::ROLE_OUTLINE) :
>-    static_cast<PRUint32>(nsIAccessibleRole::ROLE_LIST);
>+    static_cast<Role>(roles::ROLE_OUTLINE) :
>+    static_cast<Role>(roles::ROLE_LIST);

get rid of the static casts

>   return primaryColumn ?
>-    static_cast<PRUint32>(nsIAccessibleRole::ROLE_OUTLINEITEM) :
>-    static_cast<PRUint32>(nsIAccessibleRole::ROLE_LISTITEM);
>+    static_cast<Role>(roles::ROLE_OUTLINEITEM) :
>+    static_cast<Role>(roles::ROLE_LISTITEM);

same


thanks!

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> >  *  This list of WAI-defined roles are currently hardcoded.
> >  *  Eventually we will most likely be loading an RDF resource that contains this information
> >  *  Using RDF will also allow for role extensibility. See bug 280138.
> 
> Alex, are we actually at all interested in this idea?

We are interested in problem solving. I never looked seriously at the idea. But since we have a valid bug then it's ok to keep the comment if you that's a concern.

> >  *
> >  *  Definition of nsRoleMapEntry and nsStateMapEntry contains comments explaining this table.
> >  *
> >- *  When no nsIAccessibleRole enum mapping exists for an ARIA role, the
> >+ *  When no roles enum mapping exists for an ARIA role, the
> >  *  role will be exposed via the object attribute "xml-roles".
> >  *  In addition, in MSAA, the unmapped role will also be exposed as a BSTR string role.
> 
> this seems sort of platformy, Alex how about we update the whole comment?

I'd say 'for example' :) Like

When no proper accessible role exists for an ARIA role, the ARIA role is available via the object attribute "xml-roles". Platform APIs can provide additional mechanisms to expose it. For example, in MSAA, the unmapped role is also exposed as a BSTR string role.

> >  *
> >- *  There are no nsIAccessibleRole enums for the following landmark roles:
> >+ *  There are no roles enums for the following landmark roles:
> >  *    banner, contentinfo, main, navigation, note, search, secondary, seealso, breadcrumbs
> 
> do we actually think this makes sense, or should we do something like
> figure?   (I haven't dug through bugs to see)

What about the figure?

> 
> >     "timer",
> >-    nsIAccessibleRole::ROLE_NOTHING,
> >+    roles::ROLE_NOTHING,
> 
> seems a little funny no? maybe call it a platform text thing with aria-live
> like text?

can you elaborate this please?

> nsAccUtils::ROle() gets xpcom role, so maybe keep nsIAccessibleRole for now

ridiculous but correct

> >diff --git a/accessible/src/base/nsFormControlAccessible.cpp b/accessible/src/base/nsFormControlAccessible.cpp
> >  * 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 "Role.h"
> > 
> > // NOTE: alphabetically ordered
> > #include "nsFormControlAccessible.h"
> > #include "nsIDOMHTMLFormElement.h"
> > #include "nsIDOMHTMLInputElement.h"
> > #include "nsIDOMXULElement.h"
> > #include "nsIDOMXULControlElement.h"
> 
> I don't like putting Role.h first like that, but I'm not sure exactly what I
> prefer, Alex?

we keep accessible headers in separate section

> >-  PRUint32 xpRole = xpAccessible->Role();
> >+  Role xpRole = xpAccessible->Role();
> >   PRUint32 msaaRole = gWindowsRoleMap[xpRole].msaaRole;
> 
> Alex, any problem with plain role?

what is a plain role? You mean 'role' as variable name? I'm fine with it.

Comment 13

[Trevor Saunders (:tbsaunde)]

> > >  *
> > >  *  Definition of nsRoleMapEntry and nsStateMapEntry contains comments explaining this table.
> > >  *
> > >- *  When no nsIAccessibleRole enum mapping exists for an ARIA role, the
> > >+ *  When no roles enum mapping exists for an ARIA role, the
> > >  *  role will be exposed via the object attribute "xml-roles".
> > >  *  In addition, in MSAA, the unmapped role will also be exposed as a BSTR string role.
> > 
> > this seems sort of platformy, Alex how about we update the whole comment?
> 
> I'd say 'for example' :) Like
> 
> When no proper accessible role exists for an ARIA role, the ARIA role is
> available via the object attribute "xml-roles". Platform APIs can provide
> additional mechanisms to expose it. For example, in MSAA, the unmapped role
> is also exposed as a BSTR string role.

seems fine to me.

> > >  *
> > >- *  There are no nsIAccessibleRole enums for the following landmark roles:
> > >+ *  There are no roles enums for the following landmark roles:
> > >  *    banner, contentinfo, main, navigation, note, search, secondary, seealso, breadcrumbs
> > 
> > do we actually think this makes sense, or should we do something like
> > figure?   (I haven't dug through bugs to see)
> 
> What about the figure?

I mean in that case we have an internal role for it, but when exposing it to AT's we use a different role.

> > >     "timer",
> > >-    nsIAccessibleRole::ROLE_NOTHING,
> > >+    roles::ROLE_NOTHING,
> > 
> > seems a little funny no? maybe call it a platform text thing with aria-live
> > like text?
> 
> can you elaborate this please?

timer shows   the time since or until something happens right? so have the accessibles value be the time as a string, and when value changes we fire value change event.  accessible can maybe expose string as text interface too, but I'm not sure how useful that is.  We'd also call this role text field maybe because its sort of text, though maybe we should add somethingto AT  api.

> > nsAccUtils::ROle() gets xpcom role, so maybe keep nsIAccessibleRole for now
> 
> ridiculous but correct

well, once we nolonger deal with xpcom objects internal it can just go away :) I'm not sure why you think its rediculus?

> > >diff --git a/accessible/src/base/nsFormControlAccessible.cpp b/accessible/src/base/nsFormControlAccessible.cpp
> > >  * 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 "Role.h"
> > > 
> > > // NOTE: alphabetically ordered
> > > #include "nsFormControlAccessible.h"
> > > #include "nsIDOMHTMLFormElement.h"
> > > #include "nsIDOMHTMLInputElement.h"
> > > #include "nsIDOMXULElement.h"
> > > #include "nsIDOMXULControlElement.h"
> > 
> > I don't like putting Role.h first like that, but I'm not sure exactly what I
> > prefer, Alex?
> 
> we keep accessible headers in separate section

like the one that was already there but wasn't? ;)

but in general sure, but there should be a blank line after the comment.

> > >-  PRUint32 xpRole = xpAccessible->Role();
> > >+  Role xpRole = xpAccessible->Role();
> > >   PRUint32 msaaRole = gWindowsRoleMap[xpRole].msaaRole;
> > 
> > Alex, any problem with plain role?
> 
> what is a plain role? You mean 'role' as variable name? I'm fine with it.

yup

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> > > do we actually think this makes sense, or should we do something like
> > > figure?   (I haven't dug through bugs to see)
> > 
> > What about the figure?
> 
> I mean in that case we have an internal role for it, but when exposing it to
> AT's we use a different role.

we don't have internal roles for landmarks and actually landmarks aren't roles.

> > > >     "timer",
> > > >-    nsIAccessibleRole::ROLE_NOTHING,
> > > >+    roles::ROLE_NOTHING,
> > > 
> > > seems a little funny no? maybe call it a platform text thing with aria-live
> > > like text?
> > 
> > can you elaborate this please?
> 
> timer shows   the time since or until something happens right? so have the
> accessibles value be the time as a string, and when value changes we fire
> value change event.  accessible can maybe expose string as text interface
> too, but I'm not sure how useful that is.  We'd also call this role text
> field maybe because its sort of text, though maybe we should add somethingto
> AT  api.

ARIA timer doesn't override native role. It's live region so no value change events aren't required. Text interface is supported if the used element provides it. But technically this is off-topic discussion.

> > > nsAccUtils::ROle() gets xpcom role, so maybe keep nsIAccessibleRole for now
> > 
> > ridiculous but correct
> 
> well, once we nolonger deal with xpcom objects internal it can just go away
> :) I'm not sure why you think its rediculus?

because we handle in the middle, it's not XPCOM layer so we'd need to use role enum but it's correct to use XPCOM version.

> > > >+#include "Role.h"
> > > > 
> > > > // NOTE: alphabetically ordered
> > > > #include "nsFormControlAccessible.h"
> > > > #include "nsIDOMHTMLFormElement.h"
> > > > #include "nsIDOMHTMLInputElement.h"
> > > > #include "nsIDOMXULElement.h"
> > > > #include "nsIDOMXULControlElement.h"
> > > 
> > > I don't like putting Role.h first like that, but I'm not sure exactly what I
> > > prefer, Alex?
> > 
> > we keep accessible headers in separate section
> 
> like the one that was already there but wasn't? ;)
> 
> but in general sure, but there should be a blank line after the comment.

yeah, right.

Comment 15

[Jignesh Kakadiya [:jhk]]

Attachment: patch_3 part 1 in comment 4

Remaining : return types to modify . 
done : Modified part 1 in comment 4.

Comment 16

[Jignesh Kakadiya [:jhk]]

Did someone notice semicolon after while loop in nsMaiInterfaceAction.cpp line 138 ?

Comment 17

[Jignesh Kakadiya [:jhk]]

Ok my mistake.Just ignore previous comment.

Comment 18

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_4 modified part 2 . comment 4

Part 2 modified.

Comment 19

[Jignesh Kakadiya [:jhk]]

Attachment: Removed extra lines used by IFs.

patch for whole bug.

Comment 20

[Jignesh Kakadiya [:jhk]]

Attachment: Patch -> Introduced namespace roles.

.

Comment 21

[Hubert Figuiere [:hub]]

Comment on attachment 582289 [details] [diff] [review]
Patch -> Introduced namespace roles.

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

The patch fails to build on Mac.

Comment 22

[Hubert Figuiere [:hub]]

Here are the errors, at least the first batch.

In file included from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsChildView.h:45,
                 from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsCocoaUtils.mm:43:
../../../dist/include/nsAccessible.h: In member function ‘mozilla::a11y::role::Role nsAccessible::ARIARole()’:
../../../dist/include/nsAccessible.h:177: error: ‘roles’ is not a class or namespace
In file included from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsChildView.h:45,
                 from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsAppShell.mm:62:
../../../dist/include/nsAccessible.h: In member function ‘mozilla::a11y::role::Role nsAccessible::ARIARole()’:
../../../dist/include/nsAccessible.h:177: error: ‘roles’ is not a class or namespace
In file included from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsChildView.h:45,
                 from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsMenuBarX.mm:49:
../../../dist/include/nsAccessible.h: In member function ‘mozilla::a11y::role::Role nsAccessible::ARIARole()’:
../../../dist/include/nsAccessible.h:177: error: ‘roles’ is not a class or namespace
In file included from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsChildView.h:45,
                 from /Users/hub/source/mozilla/mozilla-central/widget/src/cocoa/nsCocoaWindow.mm:47:
../../../dist/include/nsAccessible.h: In member function ‘mozilla::a11y::role::Role nsAccessible::ARIARole()’:
../../../dist/include/nsAccessible.h:177: error: ‘roles’ is not a class or namespace

Comment 23

[Jignesh Kakadiya [:jhk]]

Attachment: patch-> Introduced namespace roles.

changes : 
- mac files modified.

Comment 24

[Hubert Figuiere [:hub]]

See build failure on Try server:

https://tbpl.mozilla.org/php/getParsedLog.php?id=7990472&tree=Try

Comment 25

[Trevor Saunders (:tbsaunde)]

Comment on attachment 582324 [details] [diff] [review]
patch-> Introduced namespace roles.

>+typedef mozilla::a11y::role::Role roles;

no, as I told you role, not roles, its only one thing.

nsAccessible.h

>+  typedef mozilla::a11y::role::Role roles;

same

Comment 26

[Jignesh Kakadiya [:jhk]]

If we use role in place of role there type mismatch occurs in here.

http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.h#276.

Comment 27

[Jignesh Kakadiya [:jhk]]

(In reply to Jignesh kakadiya from comment #26)
> If we use role in place of roles there type mismatch occurs in here.

Any reason why it compiles successfully in the local build which I am running and not in try server.

Comment 28

[Jignesh Kakadiya [:jhk]]

Attachment: patch-> Introduced namespace roles.

changes :
- In typedef and related changes in all files.

Comment 29

[Hubert Figuiere [:hub]]

(In reply to Jignesh kakadiya from comment #27)
> (In reply to Jignesh kakadiya from comment #26)
> > If we use role in place of roles there type mismatch occurs in here.
> 
> Any reason why it compiles successfully in the local build which I am
> running and not in try server.

TryServers use MacOS 10.6 and the associated toolchain. What do you use?

Comment 30

[Jignesh Kakadiya [:jhk]]

> TryServers use MacOS 10.6 and the associated toolchain. What do you use?

Linux.Probably that is the reason for it.

Comment 31

[Hubert Figuiere [:hub]]

(In reply to Jignesh kakadiya from comment #30)
> > TryServers use MacOS 10.6 and the associated toolchain. What do you use?
> 
> Linux.Probably that is the reason for it.

You can't check Mac code without building on a Mac. I thought that was obvious. If you don't have one that's fine. Just say so.

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #24)
> See build failure on Try server:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=7990472&tree=Try

That's kind of interesting I suppose, but I'm not particularly interested in being sure this passes try until we're a bit closer to having something ready to land.

Comment 33

[Trevor Saunders (:tbsaunde)]

Comment on attachment 582541 [details] [diff] [review]
patch-> Introduced namespace roles.


>diff --git a/accessible/src/base/nsARIAMap.h b/accessible/src/base/nsARIAMap.h> 
>+#include "mozilla/a11y/Role.h"
>+

no need forthe include you should be able to just write enum mozilla::a11y::role

>   
>   // Role mapping rule: maps to this nsIAccessibleRole

update comment

>+    if (parent && parent->Role() == roles::TREE_TABLE) {
>       // It is a row inside flatten treegrid. Group level is always 1 until it
>       // is overriden by aria-level attribute.
>       return 1;
>     }

move comment before if, and remove braces.

> 
>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -50,16 +50,17 @@
> #include "nsAccEvent.h"
> #include "nsAccessibleRelation.h"
> #include "nsAccessibilityService.h"
> #include "nsAccTreeWalker.h"
> #include "nsIAccessibleRelation.h"
> #include "nsTextEquivUtils.h"
> #include "Relation.h"
> #include "States.h"
>+#include "Role.h"


out of order

>    * Return enumerated accessible role (see constants in nsIAccessibleRole).
>    */

update

>    * Return accessible role specified by ARIA (see constants in
>    * nsIAccessibleRole).
>    */

same

>    * Returns enumerated accessible role from native markup (see constants in
>    * nsIAccessibleRole). Doesn't take into account ARIA roles.
>    */

same

>diff --git a/accessible/src/base/nsFormControlAccessible.cpp b/accessible/src/base/nsFormControlAccessible.cpp
>--- a/accessible/src/base/nsFormControlAccessible.cpp
>+++ b/accessible/src/base/nsFormControlAccessible.cpp
>@@ -39,16 +39,19 @@
> 
> // NOTE: alphabetically ordered
> #include "nsFormControlAccessible.h"
> #include "nsIDOMHTMLFormElement.h"
> #include "nsIDOMHTMLInputElement.h"
> #include "nsIDOMXULElement.h"
> #include "nsIDOMXULControlElement.h"
> 
>+#include "Role.h"

a11y headers come first


>diff --git a/accessible/src/html/nsHTMLImageAccessible.cpp b/accessible/src/html/nsHTMLImageAccessible.cpp
> 
> #include "nsHTMLImageAccessible.h"
> 
>+#include "nsAccUtils.h"
> #include "States.h"
>-#include "nsAccUtils.h"
>+#include "Role.h"

out of order

>diff --git a/accessible/src/html/nsHTMLTableAccessible.cpp 

note to self stopped here

Comment 34

[Jignesh Kakadiya [:jhk]]

Attachment: Patch (Introduced namespace role)

Comment 35

[Jignesh Kakadiya [:jhk]]

Attachment: Patch -> Introduced namespace roles.

Comment 36

[Trevor Saunders (:tbsaunde)]

Comment on attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.

r=me with comments on irc

Comment 37

[Hubert Figuiere [:hub]]

Comment on attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.

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

Does not build on MacOS. See comment on how to fix it.

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +141,5 @@
>    // TableAccessible
>    inline nsAccessible* Caption() const
>    {
>      nsAccessible* child = mChildren.SafeElementAt(0, nsnull);
> +    return child && child->Role() == role::CAPTION ? child : nsnull;

This should be roles::CAPTION instead of role::CAPTION

Comment 38

[Jignesh Kakadiya [:jhk]]

Attachment: Patch -> Introduced namespace roles.

Comment 39

[Jignesh Kakadiya [:jhk]]

Attachment: Patch -> Introduced namespace roles.

Comment 40

[Trevor Saunders (:tbsaunde)]

Attachment: windows build errors

ok, so it seems like msvc doesn't recognize role as a type.  However I don't really understand why, a minimal test case of inheriting typedefs from a parent class seems to work.  Alex David ideas?  Sadly my  best suggestion atm is virtual mozilla::a11y::role NativeRole(); which we may be able to clean up some as we namespace more stuff.

Comment 41

[Trevor Saunders (:tbsaunde)]

Comment on attachment 584154 [details] [diff] [review]
Patch -> Introduced namespace roles.

This seems fine to me, but I'd like to wait and just review the final patch that builds on windows.

Comment 42

[Trevor Saunders (:tbsaunde)]

> 
> Does not build on MacOS. See comment on how to fix it.

thats a little more interesting this time than last time, but your completely wasting your time here, this is why we have test machines.

Comment 43

[Jignesh Kakadiya [:jhk]]

Attachment: Replaced role NativeRole with mozilla::a11y::role in headers.

Comment 44

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 45

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 46

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 47

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/projects/accessibility/rev/0ef29ddd07e5
https://hg.mozilla.org/projects/accessibility/rev/90a2cc46965d

I believe Marco's going to merge to m-c today, or atleast very soon

Comment 48

[Marco Zehe (:MarcoZ)]

Merged to m-c:
http://hg.mozilla.org/mozilla-central/rev/0ef29ddd07e5
http://hg.mozilla.org/mozilla-central/rev/90a2cc46965d

Thank you for working on this, Jignesh, and also for coping with our review process! :)