Closed Bug 343137 Opened 14 years ago Closed 13 years ago

Multiple ARIA role support inconsistent with ARIA spec, hurts forward compat

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(3 files, 2 obsolete files)

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.
Blocks: aria
Blocks: keya11y
No longer blocks: fox2access
We no longer break if it's done, although we don't support multiple roles. We'll just look at the first role.
No longer blocks: keya11y
Blocks: 191a11y
No longer blocks: aria
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.
Blocks: simplearia
No longer blocks: 191a11y
Attachment #283881 - Attachment is obsolete: true
nits: please document new SetRoleMapEntry and moved GetRoleContent methods.
(In reply to comment #5)
> nits: please document new SetRoleMapEntry and moved GetRoleContent methods.
> 

only for moved one :)
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?
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?
Surkov, I just answered your question in comment 8 but you were not CC'd.
(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.
(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.
> 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.
I didn't get about wairole prefix. Is it allowed or denied to use it?
It would be nice to add EqualsLiteral method to nsRoleMapEntry, it allows to avoid  strcmp(roleMapEntry->roleString, "presentation"). It's easier to read.
(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?
(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. 

Attachment #283882 - Attachment is obsolete: true
Attachment #286243 - Flags: review?(surkov.alexander)
Attachment #283882 - Flags: review?(surkov.alexander)
comments 5 and 8 are actual still.
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
Attachment #286243 - Flags: review?(surkov.alexander) → review+
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.
Attachment #286591 - Flags: approval1.9?
Attachment #286591 - Flags: approval1.9? → superreview?(neil)
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;
  }
%}
Attachment #286591 - Flags: superreview?(neil) → superreview+
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.
Attachment #286834 - Flags: superreview?(neil)
Attachment #286834 - Flags: superreview?(neil) → superreview+
Attachment #286834 - Flags: approval1.9?
Summary: Do we need to support multiple DHTML roles? → Multiple ARIA role support inconsistent with ARIA spec, hurts forward compat
Attachment #286834 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.