Aaron Leventhal Assignee
	Description • 18 years ago

I can't remember what the working group decided on this, but at the very least we should only use the first role for now, and not break if multiple roles are specified.

	Aaron Leventhal Assignee
	Comment 1 • 18 years ago

We no longer break if it's done, although we don't support multiple roles. We'll just look at the first role.

	Aaron Leventhal Assignee
	Comment 2 • 17 years ago

Comment 1 is not true: we don't look at the first role only.

We should do whatever is finally decided in:
http://simon.html5.org/specs/aria-proposal

We'll probably recognize the first ARIA role and map that.

	alexander :surkov (:asurkov)
	Comment 5 • 17 years ago

nits: please document new SetRoleMapEntry and moved GetRoleContent methods.

	alexander :surkov (:asurkov)
	Comment 6 • 17 years ago

(In reply to comment #5)
> nits: please document new SetRoleMapEntry and moved GetRoleContent methods.
> 

only for moved one :)

	alexander :surkov (:asurkov)
	Comment 7 • 17 years ago

I don't like you call GetRoleMapEntry twice for some accessibles (from nsAccessibilityService::GetAccessible() and nsAccessible::Init()). Can you see a way to avoid this?

	Aaron Leventhal Assignee
	Comment 8 • 17 years ago

Surkov, the GetRoleMapEntry() in Init() is a mistake -- I'll remove that. We don't need it because nsAccessibilityService::InitAccessible() does SetRoleMapEntry(). Can you keep reviewing without that change?

	Aaron Leventhal Assignee
	Comment 9 • 17 years ago

Surkov, I just answered your question in comment 8 but you were not CC'd.

	alexander :surkov (:asurkov)
	Comment 10 • 17 years ago

(In reply to comment #8)
> Surkov, the GetRoleMapEntry() in Init() is a mistake -- I'll remove that. We
> don't need it because nsAccessibilityService::InitAccessible() does
> SetRoleMapEntry(). Can you keep reviewing without that change?
> 

But InitAccessible() is called with nsnull as aRoleMapEntry.

	alexander :surkov (:asurkov)
	Comment 11 • 17 years ago

(In reply to comment #9)
> Surkov, I just answered your question in comment 8 but you were not CC'd.
> 

In general I watch you, cc me if you want to get additional attention from me.

	Aaron Leventhal Assignee
	Comment 12 • 17 years ago

> But InitAccessible() is called with nsnull as aRoleMapEntry.
Only for text accessibles -- they can't have a role since they aren't elements.

At the bottom of theat method we now have this:

@@ -1446,11 +1452,11 @@ NS_IMETHODIMP nsAccessibilityService::Ge
       // Interesting generic non-HTML container
       newAcc = new nsAccessibleWrap(aNode, aWeakShell);
     }
   }
 
-  return InitAccessible(newAcc, aAccessible);
+  return InitAccessible(newAcc, aAccessible, roleMapEntry);
 }

That means all other accessibles we create will get a role map entry.

	alexander :surkov (:asurkov)
	Comment 13 • 17 years ago

I didn't get about wairole prefix. Is it allowed or denied to use it?

	alexander :surkov (:asurkov)
	Comment 14 • 17 years ago

It would be nice to add EqualsLiteral method to nsRoleMapEntry, it allows to avoid  strcmp(roleMapEntry->roleString, "presentation"). It's easier to read.

	alexander :surkov (:asurkov)
	Comment 15 • 17 years ago

(In reply to comment #13)
> I didn't get about wairole prefix. Is it allowed or denied to use it?
> 

allowed but not denied I guess.

nsAccessible::GetAttributes() can sets "xml-roles" attribute 'wairole' prefix. Is it ok?

	Aaron Leventhal Assignee
	Comment 16 • 17 years ago

(In reply to comment #14)
> It would be nice to add EqualsLiteral method to nsRoleMapEntry, it allows to
> avoid  strcmp(roleMapEntry->roleString, "presentation"). It's easier to read.
No, I like it how it is because there is no sense hiding strcmp for just one instance. We still need strcmp for the binary search because we need result of -1/0/1, not just equal/not-equal. I did change it to nsCRT::strcmp for consistency with the rest of nsAccessibilityService, though.

I'm not sure if final spec will allow author to define prefix mapping for wairole via xmlns. So, my next patch includes that capability since we have it now. It is easier to remove it later than to add it.

> nsAccessible::GetAttributes() can sets "xml-roles" attribute 'wairole' prefix.
> Is it ok?
Good point, I think it's better to trim off the prefix when the role is for the WAI roles. I have added that to the upcoming patch. 

	alexander :surkov (:asurkov)
	Comment 18 • 17 years ago

comments 5 and 8 are actual still.

	alexander :surkov (:asurkov)
	Comment 19 • 17 years ago

Comment on attachment 286243 [details] [diff] [review]
1) Still allow wairole to use xmlns prefix mapping, 2) Trim prefixes off for xml-roles when the prefix is for WAI roles

rest looks ok

	Aaron Leventhal Assignee
	Comment 20 • 17 years ago

This patch corrects an issue in ARIA support relative to http://simon.html5.org/specs/aria-proposal

Now that there is more that one browser vendor involved in ARIA, it's important that we expose ARIA in a common way.

	neil@parkwaycc.co.uk
	Comment 21 • 17 years ago

Comment on attachment 286591 [details] [diff] [review]
Patch for checkin (address comments 5 & 8)

>+%{C++
>+   virtual void SetRoleMapEntry(nsRoleMapEntry* aRoleMapEntry) = 0;
>+%}
You shouldn't add virtual methods in C++ blocks. There are a couple of options:
1. Since the interface isn't generally scriptable, simply convert the .idl into the equivalent .h file and then you can simply add this virtual method.
2. Use a native idl type, i.e.
%{C++
class nsRoleMapEntry;
%}
[ptr] native nsRoleMapEntryPtr(nsRoleMapEntry);
...
  [notxpcom] void SetRoleMapEntry(in nsRoleMapEntryPtr aRoleMapEntry);
3. Move the member into the interface (assuming that is possible):
%{C++
  nsRoleMapEntry* mRoleMapEntry;
  void SetRoleMapEntry(nsRoleMapEntry* aRoleMapEntry) {
    mRoleMapEntry = aRoleMapEntry;
  }
%}

	Aaron Leventhal Assignee
	Comment 22 • 17 years ago

Neil, how does this look? When I went with [noxpcom] I couldn't get it to build.

Note that after we branch for Firefox 3, we probably want to remove this and other nsPIAccess* interfaces and use C++ classes directly for internal operations.