Last Comment Bug 673689 - introduce namespace role
: introduce namespace role
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jignesh Kakadiya [:jhk]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: cleana11y dexpcoma11y
  Show dependency treegraph
 
Reported: 2011-07-23 04:06 PDT by alexander :surkov
Modified: 2012-01-12 05:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch_1 (212.48 KB, patch)
2011-12-08 08:55 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
patch_2 (212.97 KB, patch)
2011-12-08 09:04 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
patch_3 part 1 in comment 4 (225.75 KB, patch)
2011-12-09 03:11 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_4 modified part 2 . comment 4 (285.04 KB, patch)
2011-12-09 05:30 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Removed extra lines used by IFs. (284.76 KB, patch)
2011-12-10 06:35 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch -> Introduced namespace roles. (244.67 KB, patch)
2011-12-16 08:56 PST, Jignesh Kakadiya [:jhk]
hub: review-
Details | Diff | Splinter Review
patch-> Introduced namespace roles. (268.10 KB, patch)
2011-12-16 11:04 PST, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
patch-> Introduced namespace roles. (246.60 KB, patch)
2011-12-17 08:25 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (Introduced namespace role) (248.46 KB, patch)
2011-12-23 01:56 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch -> Introduced namespace roles. (270.52 KB, patch)
2011-12-23 10:08 PST, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review+
hub: review-
Details | Diff | Splinter Review
Patch -> Introduced namespace roles. (271.46 KB, patch)
2011-12-23 16:30 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch -> Introduced namespace roles. (290.24 KB, patch)
2011-12-23 19:00 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
windows build errors (35.12 KB, text/plain)
2012-01-06 10:48 PST, Trevor Saunders (:tbsaunde)
no flags Details
Replaced role NativeRole with mozilla::a11y::role in headers. (291.62 KB, patch)
2012-01-06 11:15 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (291.62 KB, patch)
2012-01-06 12:44 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (290.24 KB, patch)
2012-01-07 00:13 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (291.15 KB, patch)
2012-01-07 01:21 PST, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-07-23 04:06:53 PDT
role namespace similar to states namespace should contain accessible roles constants
Comment 1 alexander :surkov 2011-10-03 22:02:13 PDT
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 2011-10-03 22:09:42 PDT
Trevor, looks good for you?
Comment 3 Trevor Saunders (:tbsaunde) 2011-10-04 06:16:46 PDT
(In reply to alexander surkov from comment #2)
> Trevor, looks good for you?

SURE
Comment 4 alexander :surkov 2011-12-06 09:33:53 PST
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] 2011-12-08 08:55:35 PST
Created attachment 580062 [details] [diff] [review]
Patch_1

Need Review.
Comment 6 Jignesh Kakadiya [:jhk] 2011-12-08 09:04:18 PST
Created attachment 580066 [details] [diff] [review]
patch_2
Comment 7 Trevor Saunders (:tbsaunde) 2011-12-08 10:26:17 PST
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] 2011-12-08 11:56:07 PST
Trevor , You caught me ;).
Comment 9 Trevor Saunders (:tbsaunde) 2011-12-08 12:17:25 PST
(In reply to Jignesh kakadiya from comment #8)
> Trevor , You caught me ;).

HUH?
Comment 10 Jignesh Kakadiya [:jhk] 2011-12-08 12:31:21 PST
> (please tell me this file was just sed -i s/nsIAccessibleRole/role/)

sed s/nsIAccessibleRole/roles/g
Comment 11 Trevor Saunders (:tbsaunde) 2011-12-08 19:26:58 PST
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 2011-12-08 23:22:02 PST
(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) 2011-12-09 00:10:22 PST
> > >  *
> > >  *  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 2011-12-09 00:53:58 PST
(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] 2011-12-09 03:11:56 PST
Created attachment 580355 [details] [diff] [review]
patch_3 part 1 in comment 4

Remaining : return types to modify . 
done : Modified part 1 in comment 4.
Comment 16 Jignesh Kakadiya [:jhk] 2011-12-09 03:41:51 PST
Did someone notice semicolon after while loop in nsMaiInterfaceAction.cpp line 138 ?
Comment 17 Jignesh Kakadiya [:jhk] 2011-12-09 03:43:39 PST
Ok my mistake.Just ignore previous comment.
Comment 18 Jignesh Kakadiya [:jhk] 2011-12-09 05:30:13 PST
Created attachment 580381 [details] [diff] [review]
Patch_4 modified part 2 . comment 4

Part 2 modified.
Comment 19 Jignesh Kakadiya [:jhk] 2011-12-10 06:35:37 PST
Created attachment 580645 [details] [diff] [review]
Removed extra lines used by IFs.

patch for whole bug.
Comment 20 Jignesh Kakadiya [:jhk] 2011-12-16 08:56:56 PST
Created attachment 582289 [details] [diff] [review]
Patch -> Introduced namespace roles.

.
Comment 21 Hubert Figuiere [:hub] 2011-12-16 10:26:50 PST
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] 2011-12-16 10:51:02 PST
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] 2011-12-16 11:04:24 PST
Created attachment 582324 [details] [diff] [review]
patch-> Introduced namespace roles.

changes : 
- mac files modified.
Comment 24 Hubert Figuiere [:hub] 2011-12-16 15:53:40 PST
See build failure on Try server:

https://tbpl.mozilla.org/php/getParsedLog.php?id=7990472&tree=Try
Comment 25 Trevor Saunders (:tbsaunde) 2011-12-16 17:30:48 PST
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] 2011-12-17 02:13:32 PST
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] 2011-12-17 02:25:17 PST
(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] 2011-12-17 08:25:42 PST
Created attachment 582541 [details] [diff] [review]
patch-> Introduced namespace roles.

changes :
- In typedef and related changes in all files.
Comment 29 Hubert Figuiere [:hub] 2011-12-17 11:00:21 PST
(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] 2011-12-17 13:03:54 PST
> 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] 2011-12-17 13:10:19 PST
(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) 2011-12-17 22:15:57 PST
(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) 2011-12-21 09:03:53 PST
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] 2011-12-23 01:56:58 PST
Created attachment 584026 [details] [diff] [review]
Patch (Introduced namespace role)
Comment 35 Jignesh Kakadiya [:jhk] 2011-12-23 10:08:52 PST
Created attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.
Comment 36 Trevor Saunders (:tbsaunde) 2011-12-23 13:48:40 PST
Comment on attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.

r=me with comments on irc
Comment 37 Hubert Figuiere [:hub] 2011-12-23 14:38:09 PST
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] 2011-12-23 16:30:05 PST
Created attachment 584142 [details] [diff] [review]
Patch -> Introduced namespace roles.
Comment 39 Jignesh Kakadiya [:jhk] 2011-12-23 19:00:04 PST
Created attachment 584154 [details] [diff] [review]
Patch -> Introduced namespace roles.
Comment 40 Trevor Saunders (:tbsaunde) 2012-01-06 10:48:40 PST
Created attachment 586478 [details]
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) 2012-01-06 10:51:50 PST
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) 2012-01-06 10:56:30 PST
> 
> 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] 2012-01-06 11:15:11 PST
Created attachment 586488 [details] [diff] [review]
Replaced role NativeRole with mozilla::a11y::role in headers.
Comment 44 Jignesh Kakadiya [:jhk] 2012-01-06 12:44:26 PST
Created attachment 586524 [details] [diff] [review]
Patch
Comment 45 Jignesh Kakadiya [:jhk] 2012-01-07 00:13:43 PST
Created attachment 586648 [details] [diff] [review]
Patch
Comment 46 Jignesh Kakadiya [:jhk] 2012-01-07 01:21:16 PST
Created attachment 586650 [details] [diff] [review]
Patch
Comment 47 Trevor Saunders (:tbsaunde) 2012-01-11 22:18:34 PST
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) 2012-01-12 04:56:06 PST
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! :)

Note You need to log in before you can comment on or make changes to this bug.