Closed Bug 1137748 Opened 9 years ago Closed 9 years ago

Add and adjust roles, sub roles, and role descriptions, to be in line with WebKit's exposed stuff

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: MarcoZ, Assigned: MarcoZ)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch WIP V1 (obsolete) — Splinter Review
Alex, considering the big number of empty subroles in the ARIA map, does this justify a change this big? Or should I rather go with a sub role map in MozAccessible.mm, similar to what you did for the roleDescription, and add specific mappings for ARIA roles and the native stuff that's currently switch-cased there?
Attachment #8584661 - Flags: feedback?(surkov.alexander)
(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Created attachment 8584661 [details] [diff] [review]
> WIP V1
> 
> Alex, considering the big number of empty subroles in the ARIA map, does
> this justify a change this big? Or should I rather go with a sub role map in
> MozAccessible.mm, similar to what you did for the roleDescription, and add
> specific mappings for ARIA roles and the native stuff that's currently
> switch-cased there?

agree, 2nd approach looks nicer
Attachment #8584661 - Flags: feedback?(surkov.alexander)
This adds mappings for various ARIA roles besides landmarks which cannot be inferred from the role alone. In a second step, I plan to refactor the second switch...case statement to map roles to subroles and add more definitions if needed. Also, a few current role mappings will be corrected. But this is the first step I wanted to make sure is OK.
Assignee: nobody → mzehe
Attachment #8584661 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8585610 - Flags: feedback?(surkov.alexander)
Comment on attachment 8585610 [details] [diff] [review]
WIP Part 1: Add ARIA role mappings

Review of attachment 8585610 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/mac/mozAccessible.mm
@@ +431,5 @@
> +  const nsString& subRole;
> +};
> +
> +static const SubRoleMap sSubRoleMap[] = {
> +  { nsGkAtoms::alert, NS_LITERAL_STRING("AXApplicationAlert") },

I'm curious if it works because atoms are initialized at runtime iirc, I think that was a reason we used to keep pointers to nsGkAtoms

@@ +473,5 @@
> +    nsIAtom* ariaRole = *(mGeckoAccessible->ARIARoleMap()->roleAtom);
> +    size_t idx = 0;
> +    if (BinarySearchIf(sSubRoleMap, 0, ArrayLength(sSubRoleMap),
> +                       SubRoleComparator(ariaRole), &idx)) {
> +      return sSubRoleMap[idx].subRole;

they are different types, are they magically converted somehow? I think I would use const char* instead
Attachment #8585610 - Flags: feedback?(surkov.alexander) → feedback+
(In reply to alexander :surkov from comment #4)
> they are different types, are they magically converted somehow? I think I
> would use const char* instead

That's why I asked for feedback on the general approach, since that is the part I hadn't quite figured out yet. OK I'll just use hard-coded strings in the map, then. One question: Is there a utility method to convert these nsIAtom* thingies to strings? Since I get an nsIAtom back from the GeckoAccessible RoleMap.
Flags: needinfo?(surkov.alexander)
I think I would try something like

struct SubRoleMap
{
  nsIAtom** role;
  const char* subRole;

};

...
 { &nsGkAtoms::region, "AXDocumentRegion" },
...

if (BinarySearchIf(sSubRoleMap, 0, ArrayLength(sSubRoleMap),
    SubRoleComparator(ariaRole), &idx)) {
  return [NSString stringWithUTF8String:sSubRoleMap[idx].subRole];
}
Flags: needinfo?(surkov.alexander)
Note to self: Another source worth looking at for mappings is the test suite in Chromium: https://chromium.googlesource.com/chromium/src.git/+/master/content/test/data/accessibility/
Bug 1137748 - Adjust and correct exposed roles, subroles, and roledescriptions on OS X
Attachment #8626754 - Flags: review?(surkov.alexander)
Comment on attachment 8626754 [details]
MozReview Request: Bug 1137748 - Adjust and correct exposed roles, subroles, and roledescriptions on OS X

https://reviewboard.mozilla.org/r/12137/#review10621

::: accessible/mac/mozAccessible.mm:733
(Diff revision 1)
> -  nsIAtom* landmark = accWrap->LandmarkRole();
> +  if (accWrap->HasARIARole()) {

LandmarkRole is may be provided by non ARIA content, so you need to keep it and put HasARIARole() thing separately
Attachment #8626754 - Flags: review?(surkov.alexander)
Bug 1137748 - Expose correct roles, subroles, and roledescriptions for various WAI-ARIA roles, r?surkov
Attachment #8626838 - Flags: review?(surkov.alexander)
Attachment #8626838 - Flags: review?(surkov.alexander)
Comment on attachment 8626838 [details]
MozReview Request: Bug 1137748 - Expose correct roles, subroles, and roledescriptions for various WAI-ARIA roles, r?surkov

https://reviewboard.mozilla.org/r/12165/#review10627

just curious if these two changeset can be merged into one?
(In reply to alexander :surkov from comment #11)
> just curious if these two changeset can be merged into one?

Yes, I will do that when rebasing my "branch", the bookmark, onto the latest inbound before pushing.
Attachment #8585610 - Attachment is obsolete: true
Comment on attachment 8626838 [details]
MozReview Request: Bug 1137748 - Expose correct roles, subroles, and roledescriptions for various WAI-ARIA roles, r?surkov

https://reviewboard.mozilla.org/r/12165/#review10631

Ship It!
Attachment #8626838 - Flags: review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4f428037b5c4dbce6cdbbeea9a3afb43a2e882
changeset:  0d4f428037b5c4dbce6cdbbeea9a3afb43a2e882
user:       Marco Zehe <mzehe@mozilla.com>
date:       Fri Jun 26 17:31:44 2015 -0700
description:
Bug 1137748 - Expose correct roles, subroles, and roledescriptions for various WAI-ARIA roles on OS X, r=surkov
https://hg.mozilla.org/mozilla-central/rev/0d4f428037b5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: