Most ARIA usage is actually occuring in text/html. Because the namespace for a QName prefix cannot be defined, we require a fake prefix of "wairole:" before WAI role names. However, this is just 8 extra nonsense characters that don't mean anything, and simply makes ARIA more difficult to use. It does not make sense to require a namespaced prefix for role use in text/html. IOW we should allow: role="menuitem" directly. The old method of using role="wairole:menuitem" should still work.

Older testcases that still work with this patch: 1) http://www.mozilla.org/access/dhtml/checkbox -- xhtml 2) http://www.mozilla.org/access/dhtml/class/checkbox -- html, but uses fake "wairole:" prefix

Comment on attachment 280610 [details] [diff] [review] Correct patch: 1) Clean up code to get or compare ARIA role for a given node, 2) Have it return role name w/o prefix if it is a WAI Role, 3) Optimize so that nsIDOM3Node is not QI'd to in text/html >+ /** >+ * Does the current content have this ARIA role? >+ * @param aContent Node to get role string from >+ * @param aRoleName Role string to compare with >+ * @return PR_TRUE if there is a match >+ */ >+ static PRBool ARIARoleEquals(nsIContent *aContent, char *aRoleName) >+ { nsAutoString role; return GetARIARole(aContent, role) && role.EqualsLiteral("menu"); } Why are we checking for literal "menu"? Maybe you mean't to check against aRoleName?

Comment on attachment 280644 [details] [diff] [review] Fix ARIARoleEquals() -- good catch David Bolter -- preserve optimization (compile type calculation of const string length) Also, to test new ARIARoleEquals() I used the following: http://www.mozilla.org/access/dhtml/alert and http://www.mozilla.org/access/dhtml/spreadsheet (the menu)

Comment on attachment 280644 [details] [diff] [review] Fix ARIARoleEquals() -- good catch David Bolter -- preserve optimization (compile type calculation of const string length) Looks clean to me now. Will be nice to see this patch go in.

Comment on attachment 280644 [details] [diff] [review] Fix ARIARoleEquals() -- good catch David Bolter -- preserve optimization (compile type calculation of const string length) > nsAutoString role; >- if (nsAccessNode::GetRoleAttribute(content, role) && >- StringEndsWith(role, NS_LITERAL_STRING(":presentation")) && >- !content->IsFocusable()) { >+ if (ARIARoleEquals(content, "presentation") && !content->IsFocusable()) { you do not initialize role anymore but it is used at 1443 line.

Comment on attachment 280644 [details] [diff] [review] Fix ARIARoleEquals() -- good catch David Bolter -- preserve optimization (compile type calculation of const string length) >Index: accessible/src/base/nsAccessNode.h > >+#define ARIARoleEquals(aContent, aRoleName) \ >+ nsAccessNode::ARIARoleEqualsImpl(aContent, aRoleName, NS_ARRAY_LENGTH(aRoleName) - 1) >+ possibly I would prefer to have special version of GetARIARole() to use it GetARIARole(content).EqualsLiteral("menu"); or modification of your way (if it works :)) static PRBool ARIARoleEquals(nsIContent* aContent, const char* aRoleName) >+ { nsAutoString role; return GetARIARole(aContent, role) && role.EqualsLiteral(aRoleName); }

> possibly I would prefer to have special version of GetARIARole() to use it > GetARIARole(content).EqualsLiteral("menu"); You would have to create a global nsAutoString to store stuff in, right? That would make some of our |if| statements less readable. Also I would have to check .IsEmpty() in more places. > or modification of your way (if it works :)) > static PRBool ARIARoleEquals(nsIContent* aContent, const char* aRoleName) > >+ { nsAutoString role; return GetARIARole(aContent, role) && > role.EqualsLiteral(aRoleName); } That doesn't work, it doesn't compile, because EqualsLiteral() needs to be passed the actual string. That's why I did it as a macro.

(In reply to comment #12) > > possibly I would prefer to have special version of GetARIARole() to use it > > GetARIARole(content).EqualsLiteral("menu"); > You would have to create a global nsAutoString to store stuff in, right? That > would make some of our |if| statements less readable. Also I would have to > check .IsEmpty() in more places. I thought about: nsAString GetARIARole(nsIContent *aContent) { nsAutoString role; GetARIARole(aContent, role); return role; } GetARIARole(content).EqualsLiteral("menu");

Comment on attachment 280728 [details] [diff] [review] Fix nsAccessibilityService() usage r=me

re: comment 14, it would no longer be on the stack, so it's destroyed by the time you do the comparison.

Actually, I'm not sure about that.

I guess it should be ok but it brings unnecessary string copy.