Closed Bug 1137714 Opened 5 years ago Closed 5 years ago

Make roleDescription nicer/correct/faster

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

the MozAccessiblemm roleDescription method is quite a mess. Make it nicer, more correct, and faster.
Blocks: osxa11y
No longer blocks: maca11y
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Attachment #8570652 - Flags: review?(mzehe)
Comment on attachment 8570652 [details] [diff] [review]
patch

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

r=me with nits fixed/questions answered.

::: accessible/mac/mozAccessible.mm
@@ +450,1 @@
>        return @"AXLandmarkSearch";

I briefly wondered if we should make these into a map, too, but then figured that the list of landmarks doesn't change that much, so it's probably not worth it, and also not less code, or?

@@ +462,2 @@
>      case roles::SWITCH:
>        return @"AXSwitch";

Same here. Would a map ake sense here, too, esp when extending with new roles?

@@ +470,5 @@
>  }
>  
> +struct RoleDescrMap
> +{
> +  NSString* role;

Nit: subRole, since we're comparing against sub roles and not the actual roles.

@@ +483,5 @@
> +  { @"AXLandmarkMain", NS_LITERAL_STRING("main") },
> +  { @"AXLandmarkNavigation", NS_LITERAL_STRING("navigation") },
> +  { @"AXLandmarkSearch", NS_LITERAL_STRING("search") },
> +  { @"AXTerm", NS_LITERAL_STRING("term") }
> +};

I'm afraid you'll have to rebase and augment the patch since I just landed bug 1121518 which adds stuff to the old code blocks in this file. Sorry! :)
Attachment #8570652 - Flags: review?(mzehe) → review+
(In reply to Marco Zehe (:MarcoZ) from comment #2)

> ::: accessible/mac/mozAccessible.mm
> @@ +450,1 @@
> >        return @"AXLandmarkSearch";
> 
> I briefly wondered if we should make these into a map, too, but then figured
> that the list of landmarks doesn't change that much, so it's probably not
> worth it, and also not less code, or?

it's small, we shouldn't benefit much from map

> @@ +462,2 @@
> >      case roles::SWITCH:
> >        return @"AXSwitch";
> 
> Same here. Would a map ake sense here, too, esp when extending with new
> roles?

I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I think

> > +struct RoleDescrMap
> > +{
> > +  NSString* role;
> 
> Nit: subRole, since we're comparing against sub roles and not the actual
> roles.

I would keep generic 'role', since it's used from roleDescription which works both for roles and subroles and (maybe quite unlikely) can be used for roles too. On the another hand subrole is also role, it's just an approach of role taxonomy implementation.

> > +  { @"AXLandmarkMain", NS_LITERAL_STRING("main") },
> > +  { @"AXLandmarkNavigation", NS_LITERAL_STRING("navigation") },
> > +  { @"AXLandmarkSearch", NS_LITERAL_STRING("search") },
> > +  { @"AXTerm", NS_LITERAL_STRING("term") }
> > +};
> 
> I'm afraid you'll have to rebase and augment the patch since I just landed
> bug 1121518 which adds stuff to the old code blocks in this file. Sorry! :)

I'm aware of that :)
(In reply to alexander :surkov from comment #3)
> (In reply to Marco Zehe (:MarcoZ) from comment #2)
> > @@ +462,2 @@
> > >      case roles::SWITCH:
> > >        return @"AXSwitch";
> > 
> > Same here. Would a map ake sense here, too, esp when extending with new
> > roles?
> 
> I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I
> think

That's what I was thinking, too. Should that be part of bug 1137748, or a separate bug, or are you going to weave that into this one?

> > > +struct RoleDescrMap
> > > +{
> > > +  NSString* role;
> > 
> > Nit: subRole, since we're comparing against sub roles and not the actual
> > roles.
> 
> I would keep generic 'role', since it's used from roleDescription which
> works both for roles and subroles and (maybe quite unlikely) can be used for
> roles too. On the another hand subrole is also role, it's just an approach
> of role taxonomy implementation.

OK fine. :)
(In reply to Marco Zehe (:MarcoZ) from comment #4)

> > I'd say this one should go into RoleMap.h, that be useful for bug 1137748 I
> > think
> 
> That's what I was thinking, too. Should that be part of bug 1137748, or a
> separate bug, or are you going to weave that into this one?

it's relatively small change and can be part of bug 1137748 I think. It can be dealt separately of course. I wouldn't grow this one :)
Fair enough. Happy landing! :)
https://hg.mozilla.org/mozilla-central/rev/5c47eaffd147
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.