742659 - move nsARIAMap::UniversalStatesFor to aria namespace

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) introduce mozilla::a11y::aria namespace in nsARIAMap.h (if it's still missed)
2) move nsARIAMap::UniversalStatesFor there
3) remove nsARIAMap::gWAIUnivStateMap and keep it as static in nsARIAMap.cpp

Comment 1

[alexander :surkov (:asurkov)]

Trevor, that makes us to not keep UniversalStatesFor inline anymore. Also following this we should replace gEmptyRoleMap on something like aria::EmptyRole() which is not inline too. I wonder how we can workaround that.

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> Trevor, that makes us to not keep UniversalStatesFor inline anymore. Also

I have no guess what that'll do to perf, but I suspect its not critical.  Otherwise why is this interesting?

> following this we should replace gEmptyRoleMap on something like
> aria::EmptyRole() which is not inline too. I wonder how we can workaround
> that.

why can't we have 

namespace aria {
nsRoleMap* gEmptyRole;
}

then in nsARIAMap.cpp

nsRoleMap sEmptyMappingRule {
...
};

gEmptyRole = &sEmptyRule;

Comment 3

[Trevor Saunders (:tbsaunde)]

> > following this we should replace gEmptyRoleMap on something like
> > aria::EmptyRole() which is not inline too. I wonder how we can workaround
> > that.
> 
> why can't we have 
> 
> namespace aria {
> nsRoleMap* gEmptyRole;
> }
> 
> then in nsARIAMap.cpp
> 
> nsRoleMap sEmptyMappingRule {
> ...
> };
> 
> gEmptyRole = &sEmptyRule;

on the other hand it would be nice if we could store an index in accessibles instead of a pointer to use less space for the same amount of information.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> why can't we have 
> 
> namespace aria {
> nsRoleMap* gEmptyRole;
> }

to keep it unified like
namespace aria {
  RoleMap* RoleMapFor(nsINode* aNode);
  RoleMap* EmptyRoleMap();
}

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> on the other hand it would be nice if we could store an index in accessibles
> instead of a pointer to use less space for the same amount of information.

well, we could do that but also that means we need to get RoleMap every time what sound not so nice as having a pointer.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > why can't we have 
> > 
> > namespace aria {
> > nsRoleMap* gEmptyRole;
> > }
> 
> to keep it unified like
> namespace aria {
>   RoleMap* RoleMapFor(nsINode* aNode);
>   RoleMap* EmptyRoleMap();

I guess that's a fair point though my feeling is sort of that's a bit far to the everything must be uniform side.

> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > on the other hand it would be nice if we could store an index in accessibles
> > instead of a pointer to use less space for the same amount of information.
> 
> well, we could do that but also that means we need to get RoleMap every time
> what sound not so nice as having a pointer.

Well, I'm not sure I understand this we either have null or we have a pointer, similarly we could have a special none number or an actual index.  After all in a sense all a pointer is an index :)

Comment 6

[Trevor Saunders (:tbsaunde)]

Alex, it occurs to me it might be more reasonable to move this to ARIAStateMap.cpp while we're here to keep the aria state stuff all in one place.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Alex, it occurs to me it might be more reasonable to move this to
> ARIAStateMap.cpp while we're here to keep the aria state stuff all in one
> place.

I'm not sure. It seems ARIAStateMap defines set of simple rules how to map ARIA states and it doesn't maintain any connection to markup used to turn on that ARIA state. nsARIAMap is more high level logic that describes connection between roles and ARIA states so perhaps it's reasonable to make nsARIAMap to describe universal roles as well.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> Well, I'm not sure I understand this we either have null or we have a
> pointer, similarly we could have a special none number or an actual index. 
> After all in a sense all a pointer is an index :)

anyway, having pointer or pointer + idx doesn't make big difference. If we provide set of inlines to operate with RoleMap by RoleMap index then it's fine with me I think. Can you think of how we can split this work into small bugs?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > Alex, it occurs to me it might be more reasonable to move this to
> > ARIAStateMap.cpp while we're here to keep the aria state stuff all in one
> > place.
> 
> I'm not sure. It seems ARIAStateMap defines set of simple rules how to map
> ARIA states and it doesn't maintain any connection to markup used to turn on
> that ARIA state. nsARIAMap is more high level logic that describes
> connection between roles and ARIA states so perhaps it's reasonable to make
> nsARIAMap to describe universal roles as well.

ok, that sort of makes sense, on the other hand I sort of tempted to try and break what's left in nsARIAMap.cpp up into related chunks.


(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > Well, I'm not sure I understand this we either have null or we have a
> > pointer, similarly we could have a special none number or an actual index. 
> > After all in a sense all a pointer is an index :)
> 
> anyway, having pointer or pointer + idx doesn't make big difference. If we
> provide set of inlines to operate with RoleMap by RoleMap index then it's
> fine with me I think. Can you think of how we can split this work into small
> bugs?

I don't have a clear picture of what I want to do in my mind yet.  I think I want to move the ARIA stuffs in nsAccUtils somewhere else, I'd sort of like to break up or find a better name for nsARIAMap, and maybe something else I'll think of as we go along, but I'm not sure I see how either of those should be done.  What do you want?

Comment 10

[alexander :surkov (:asurkov)]

I think I would replace nsARIAMap on aria namespace and then probably add helper functions to nsRoleMapEntry to move out the ARIA logic from nsAccessible (or if we keep integer instead of pointer to nsRoleMapEntry provide set of functions to deal with ARIA role). But first of all I would like to make nsARIAMap code consistent with code style we use. I think I'm ok if we follow comment #0 for this bug.

Comment 11

[pcheng [:pcheng]]

Attachment: Bug 742659

Please review.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 617180 [details] [diff] [review]
Bug 742659

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

asking Trevor for review since he had conerns

::: accessible/src/base/nsARIAMap.h
@@ +242,5 @@
> +/**
> + * Return accessible state from ARIA universal states applied to the given
> + * element.
> + */
> +static PRUint64 UniversalStatesFor(mozilla::dom::Element* aElement);

don't make it static please, if you have linker error then put aria:: to function definition in cpp

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 617180 [details] [diff] [review]
Bug 742659

>-EStateRule nsARIAMap::gWAIUnivStateMap[] = {
>+static EStateRule gWAIUnivStateMap[] = {

rename it to sWAIUnivStateMap, and make it const please

>+static PRUint64 UniversalStatesFor(mozilla::dom::Element* aElement)
>+{
>+  PRUint64 state = 0;
>+  PRUint32 index = 0;
>+  while (MapToState(gWAIUnivStateMap[index], aElement, &state))
>+    index++;

so, one thing we could do is move the array into the function as a static local var, and do
for (int idx = 0; idx < ArrayLength(sWAIUnivStateMap); idx++)
    MapToStates(elem, sWAIUnivStateMap[idx], &states);

  return states;
}

but that seem complicated when it's really just
MapToStates(elem, eARIAFoo1, &states);
MapToStates(elem, eARIAFoo2, &states);
...
for 6 values of eARIAFoo, and I'll bet the compiler just generates a list of calls.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> so, one thing we could do is move the array into the function as a static
> local var, and do
> for (int idx = 0; idx < ArrayLength(sWAIUnivStateMap); idx++)
>     MapToStates(elem, sWAIUnivStateMap[idx], &states);
> 
>   return states;
> }
> 
> but that seem complicated when it's really just
> MapToStates(elem, eARIAFoo1, &states);
> MapToStates(elem, eARIAFoo2, &states);
> ...
> for 6 values of eARIAFoo, and I'll bet the compiler just generates a list of
> calls.

I would leave it as is, perhaps a follow up would be nice (I'm not sure which way I like more).

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 617180 [details] [diff] [review]
> Bug 742659
> 
> >-EStateRule nsARIAMap::gWAIUnivStateMap[] = {
> >+static EStateRule gWAIUnivStateMap[] = {
> 
> rename it to sWAIUnivStateMap, and make it const please

pcheng, mind doing this?

Comment 16

[Mark Capella [:capella]]

Attachment: Patch - Rebased

Trev, this has been sitting for a few months ... I rebased it and addressed the last nit plus a build error ... if the review+ is still valid I can land it for the contributor...

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #16)
> Created attachment 640144 [details] [diff] [review]
> Patch - Rebased
> 
> Trev, this has been sitting for a few months ... I rebased it and addressed
> the last nit plus a build error ... if the review+ is still valid I can land
> it for the contributor...

sure, but fix the commit message / author :)

Comment 18

[Mark Capella [:capella]]

Start with TRY:
https://tbpl.mozilla.org/?tree=Try&rev=d2ef36979527

Comment 19

[Mark Capella [:capella]]

And on to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a86157211faa

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a86157211faa