introduce namespace role

RESOLVED FIXED in mozilla12

Status

()

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

People

(Reporter: surkov, Assigned: jhk)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla12
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 15 obsolete attachments)

35.12 KB, text/plain
Details
291.15 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
role namespace similar to states namespace should contain accessible roles constants
(Reporter)

Comment 1

6 years ago
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
  }
}
(Reporter)

Comment 2

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

SURE
(Reporter)

Comment 4

6 years ago
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.
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++]
(Reporter)

Updated

6 years ago
Assignee: nobody → jigneshhk1992
(Assignee)

Comment 5

6 years ago
Created attachment 580062 [details] [diff] [review]
Patch_1

Need Review.
Attachment #580062 - Flags: review?(surkov.alexander)
(Assignee)

Comment 6

6 years ago
Created attachment 580066 [details] [diff] [review]
patch_2
Attachment #580062 - Attachment is obsolete: true
Attachment #580062 - Flags: review?(surkov.alexander)
Attachment #580066 - Flags: review?(surkov.alexander)
(Reporter)

Updated

6 years ago
Attachment #580066 - Flags: review?(surkov.alexander) → review?(trev.saunders)
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
(Assignee)

Comment 8

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

HUH?
(Assignee)

Comment 10

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

sed s/nsIAccessibleRole/roles/g
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!
(Reporter)

Comment 12

6 years ago
(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.
> > >  *
> > >  *  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
(Reporter)

Comment 14

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

Comment 15

6 years ago
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.
Attachment #580066 - Attachment is obsolete: true
Attachment #580066 - Flags: review?(trev.saunders)
Attachment #580355 - Flags: review?(surkov.alexander)
(Assignee)

Comment 16

6 years ago
Did someone notice semicolon after while loop in nsMaiInterfaceAction.cpp line 138 ?
(Assignee)

Comment 17

6 years ago
Ok my mistake.Just ignore previous comment.
(Assignee)

Comment 18

6 years ago
Created attachment 580381 [details] [diff] [review]
Patch_4 modified part 2 . comment 4

Part 2 modified.
Attachment #580355 - Attachment is obsolete: true
Attachment #580355 - Flags: review?(surkov.alexander)
Attachment #580381 - Flags: review?(surkov.alexander)
(Assignee)

Comment 19

6 years ago
Created attachment 580645 [details] [diff] [review]
Removed extra lines used by IFs.

patch for whole bug.
Attachment #580381 - Attachment is obsolete: true
Attachment #580381 - Flags: review?(surkov.alexander)
Attachment #580645 - Flags: review?(surkov.alexander)
(Assignee)

Comment 20

6 years ago
Created attachment 582289 [details] [diff] [review]
Patch -> Introduced namespace roles.

.
Attachment #580645 - Attachment is obsolete: true
Attachment #580645 - Flags: review?(surkov.alexander)
Attachment #582289 - Flags: review?(trev.saunders)
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.
Attachment #582289 - Flags: review-
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
(Assignee)

Comment 23

6 years ago
Created attachment 582324 [details] [diff] [review]
patch-> Introduced namespace roles.

changes : 
- mac files modified.
Attachment #582289 - Attachment is obsolete: true
Attachment #582289 - Flags: review?(trev.saunders)
Attachment #582324 - Flags: review?(trev.saunders)
See build failure on Try server:

https://tbpl.mozilla.org/php/getParsedLog.php?id=7990472&tree=Try
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
Attachment #582324 - Flags: review?(trev.saunders) → review-
(Assignee)

Comment 26

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

Comment 27

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

Comment 28

6 years ago
Created attachment 582541 [details] [diff] [review]
patch-> Introduced namespace roles.

changes :
- In typedef and related changes in all files.
Attachment #582324 - Attachment is obsolete: true
Attachment #582541 - Flags: review?(trev.saunders)
(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?
(Assignee)

Comment 30

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

Linux.Probably that is the reason for it.
(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.
(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 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
(Assignee)

Comment 34

6 years ago
Created attachment 584026 [details] [diff] [review]
Patch (Introduced namespace role)
Attachment #582541 - Attachment is obsolete: true
Attachment #582541 - Flags: review?(trev.saunders)
Attachment #584026 - Flags: review?(trev.saunders)
(Assignee)

Comment 35

6 years ago
Created attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.
Attachment #584026 - Attachment is obsolete: true
Attachment #584026 - Flags: review?(trev.saunders)
Attachment #584082 - Flags: review?(trev.saunders)
Comment on attachment 584082 [details] [diff] [review]
Patch -> Introduced namespace roles.

r=me with comments on irc
Attachment #584082 - Flags: review?(trev.saunders) → review+
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
Attachment #584082 - Flags: review-
(Assignee)

Comment 38

6 years ago
Created attachment 584142 [details] [diff] [review]
Patch -> Introduced namespace roles.
Attachment #584082 - Attachment is obsolete: true
Attachment #584142 - Flags: review?(trev.saunders)
(Assignee)

Comment 39

6 years ago
Created attachment 584154 [details] [diff] [review]
Patch -> Introduced namespace roles.
Attachment #584142 - Attachment is obsolete: true
Attachment #584142 - Flags: review?(trev.saunders)
Attachment #584154 - Flags: review?(trev.saunders)
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 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.
Attachment #584154 - Flags: review?(trev.saunders)
> 
> 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.
(Assignee)

Comment 43

6 years ago
Created attachment 586488 [details] [diff] [review]
Replaced role NativeRole with mozilla::a11y::role in headers.
Attachment #584154 - Attachment is obsolete: true
Attachment #586488 - Flags: review?(trev.saunders)
(Assignee)

Comment 44

6 years ago
Created attachment 586524 [details] [diff] [review]
Patch
Attachment #586488 - Attachment is obsolete: true
Attachment #586488 - Flags: review?(trev.saunders)
Attachment #586524 - Flags: review?(trev.saunders)
(Assignee)

Comment 45

6 years ago
Created attachment 586648 [details] [diff] [review]
Patch
Attachment #586524 - Attachment is obsolete: true
Attachment #586524 - Flags: review?(trev.saunders)
Attachment #586648 - Flags: review?(trev.saunders)
(Assignee)

Comment 46

6 years ago
Created attachment 586650 [details] [diff] [review]
Patch
Attachment #586648 - Attachment is obsolete: true
Attachment #586648 - Flags: review?(trev.saunders)
Attachment #586650 - Flags: review?(trev.saunders)
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
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! :)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla12
Attachment #586650 - Flags: review?(trev.saunders) → review+
You need to log in before you can comment on or make changes to this bug.