475006 - Extend nsARIAMap or create hashtable to capture ARIA attribute characteristics

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

There are times we check aria attributes to discern when and how they are used, for example: nsAccUtils::IsARIAPropForObjectAttr, and like in bug 473164 where we must clean/clean token based attrs with literal value "undefined".

We can keep adding large if statements, or we can create a data structure where we can check the attr characteristic directly.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

OK I went off and did (too) much thinking and code reading for this one. I've talked to some devs and our binary searching of the ARIA map is sufficient (for now).

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP - for discussion

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 359060 [details] [diff] [review]
WIP - for discussion

>+#define ARIA_OBJ_ATTR 0x0001
>+#define ARIA_TOKEN 0x0010

Do we want to put comments here to explain a little what these values mean?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP2 - shows code we can remove.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Note this also removes the need to add code from on patch bug 473164 (i.e. I fix that bug here too)

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 359110 [details] [diff] [review]
WIP2 - shows code we can remove.

>+struct nsAttributeCharacteristics
>+{
>+  nsIAtom** attributeName;  // nsnull indicates last entry

Would perhaps be more appropriate to put the constant kEndCharacteristicArray here. After all you define it for this purpose.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> (From update of attachment 359110 [details] [diff] [review])
> >+struct nsAttributeCharacteristics
> >+{
> >+  nsIAtom** attributeName;  // nsnull indicates last entry
> 
> Would perhaps be more appropriate to put the constant kEndCharacteristicArray
> here. After all you define it for this purpose.

Good point, but I'm following a precedent. gWAIRoleMap defines kEndEntry in the cpp file.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 359110 [details] [diff] [review]
WIP2 - shows code we can remove.

Surkov, what do you think of this approach? It is 1 O(n) lookup for a small n, and then we have a bitflag for checking attribute characteristics. I could make it O(logn) but I don't think we win anything because of the need to convert nsIAtoms to strings to do a compare.

Comment 9

[alexander :surkov (:asurkov)]

I think I like it. Now we have O(n) algorithm in IsARIAPropForObjectAttr, here we increase n. It should be ok. I'll add few comments for the wip patch.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 359110 [details] [diff] [review]
WIP2 - shows code we can remove.


>+static const nsAttributeCharacteristics kEndCharacteristicArray = {nsnull, 0};
>+
>+nsAttributeCharacteristics nsARIAMap::gAttributeCharacteristics[] = {

gWAIUnivAttrMap ? On one hand I like your attribute+characteristics, on another hand I like to save accepted name notation.


>+  {&nsAccessibilityAtoms::aria_droppable,         0                             },

is there particular reason to keep 0 in the array? If there is then I prefer to have constant for it.

>+// attribute characteristic masks
>+#define ARIA_OBJ_ATTR 0x0001
>+#define ARIA_TOKEN 0x0010

enum?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: addresses surkovs comments.

How's this? I'm not terribly fond of the cast to enum type but it is safe enough. (C++ doesn't allow implicit cast from int to enum)

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Created an attachment (id=359557) [details]
> addresses surkovs comments.
> 
> How's this? I'm not terribly fond of the cast to enum type but it is safe
> enough. (C++ doesn't allow implicit cast from int to enum)

We could introduce constant for eAttrExposeObj | eAttrValToken to avoid this if I get right.

Would be nice to get answers on two comments from previous post.

enums should be documented

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP - for discussion

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: proposed fix.

Hi Surkov, I just noticed one of your comments that I hadn't addressed. This patch adds ATTR_NOFLAG to be used in place of 0. Also note I'm using your suggested naming: gWAIUnivAttrMap.

Comment 15

[alexander :surkov (:asurkov)]

David, you still didn't address one my question from comment #10: why do we need to keep ARIA attributes with ATTR_NOFLAG in the array?

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: removed 0/ATTR_NOFLAG

Surkov, sorry about that. I was taking the wrong meaning from your comment 10. I think this is a better patch, and is probably what you were looking for.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 360313 [details] [diff] [review]
removed 0/ATTR_NOFLAG

I realize this comment is a bit late because you have this patch but. If search in hash is really O(1) then we could introduce hash here instead of array. Advantages are:
1) we hold information about attribute type in unique place
2) we can get rid calls like 
nsAccUtils::HasDefinedARIAToken(ancestor, nsAccessibilityAtoms::aria_relevant) &&
ancestor->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_relevant, relevant))

you introduced in bug 452388 if we will have GetARIAAttr method.


Another common comment - please add mochitests for this (for example, create test_attrs_aria.html)


>+ * ARIA attribute map for attribute characteristics
>+ * 
>+ * ARIA attributes that don't have any flags are not included here.

nit: @note would be good here.

>+ */
>+nsAttributeCharacteristics nsARIAMap::gWAIUnivAttrMap[] = {
>+  {&nsAccessibilityAtoms::aria_activedescendant,  ATTR_EXPOSEOBJ                 },
>+  {&nsAccessibilityAtoms::aria_atomic,                             ATTR_VALTOKEN },

ARIA atoms with characteristics == ATTR_VALTOKEN (note, not characteristics == ATTR_VALTOKEN | ATTR_VALTOKEN) aren't used in this patch. How do you want to use them? I realize it's possibly bug 473164 but I want to be sure and like to know usecase.

>+  {&nsAccessibilityAtoms::aria_expanded,                           ATTR_VALTOKEN },

expanded is not object attribute any more

>+// ARIA attribute characteristic masks, grow as needed
>+const unsigned short ATTR_EXPOSEOBJ  = 0x0001; // should be exposed as an object attribute
>+const unsigned short ATTR_VALTOKEN   = 0x0010; // should have a token or bool value

nit: I like to see these commented more well, please add examples where it is used. For example

// should be exposed as an object attribute
=>
/**
  * Indicates ARIA attribute should be exposed as an object attribute (see nsAccessible::GetAttributes() method)

>+
>+// Small footprint storage of persistent aria attribute characteristics
>+struct nsAttributeCharacteristics
>+{
>+  nsIAtom** attributeName;
>+  const unsigned short characteristics;

is there a reason you use unsigned short instead of PR.. ? For exaple PRUint16 or something?

> 
>+unsigned short
>+nsAccUtils::GetAttributeCharacteristics(nsIAtom* aAtom)
>+{
>+    for (PRUint32 i; i < nsARIAMap::gWAIUnivAttrMapLength; i++)

I really doubt this works - not initialized local variable 'i' - that's the reason we strongly need mochitests :)

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #17)

Thanks Surkov. Some answers:

> (From update of attachment 360313 [details] [diff] [review])
> I realize this comment is a bit late because you have this patch but. If search
> in hash is really O(1) then we could introduce hash here instead of array.

Hash should always be O(1). I was leaning toward a hash, but I chose an array after discussing with devs on #developers. We would need a larger n for a hash to be worthwhile. I think we can always do that later if we want to, maybe after we do some profiling.

> Advantages are:
> 1) we hold information about attribute type in unique place
> 2) we can get rid calls like 
> nsAccUtils::HasDefinedARIAToken(ancestor, nsAccessibilityAtoms::aria_relevant)
> &&
> ancestor->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_relevant,
> relevant))
> 
> you introduced in bug 452388 if we will have GetARIAAttr method.

Sure, but that sounds like a separate bug?

> 
> 
> Another common comment - please add mochitests for this (for example, create
> test_attrs_aria.html)

OK

> 
> 
> >+ * ARIA attribute map for attribute characteristics
> >+ * 
> >+ * ARIA attributes that don't have any flags are not included here.
> 
> nit: @note would be good here.

Thanks.

> 
> >+ */
> >+nsAttributeCharacteristics nsARIAMap::gWAIUnivAttrMap[] = {
> >+  {&nsAccessibilityAtoms::aria_activedescendant,  ATTR_EXPOSEOBJ                 },
> >+  {&nsAccessibilityAtoms::aria_atomic,                             ATTR_VALTOKEN },
> 
> ARIA atoms with characteristics == ATTR_VALTOKEN (note, not characteristics ==
> ATTR_VALTOKEN | ATTR_VALTOKEN) aren't used in this patch. How do you want to
> use them? I realize it's possibly bug 473164 but I want to be sure and like to
> know usecase.

Can you rephrase? In bug 473164 will check only ATTR_VALTOKEN attributes so that we don't expose them if they have value "undefined" -- as per spec.

> 
> >+  {&nsAccessibilityAtoms::aria_expanded,                           ATTR_VALTOKEN },
> 
> expanded is not object attribute any more

But it does take token values (bool) right?

> 
> >+// ARIA attribute characteristic masks, grow as needed
> >+const unsigned short ATTR_EXPOSEOBJ  = 0x0001; // should be exposed as an object attribute
> >+const unsigned short ATTR_VALTOKEN   = 0x0010; // should have a token or bool value
> 
> nit: I like to see these commented more well, please add examples where it is
> used. For example
> 
> // should be exposed as an object attribute
> =>
> /**
>   * Indicates ARIA attribute should be exposed as an object attribute (see
> nsAccessible::GetAttributes() method)

OK.

> 
> >+
> >+// Small footprint storage of persistent aria attribute characteristics
> >+struct nsAttributeCharacteristics
> >+{
> >+  nsIAtom** attributeName;
> >+  const unsigned short characteristics;
> 
> is there a reason you use unsigned short instead of PR.. ? For exaple PRUint16
> or something?

Yeah I'll change it.

> 
> > 
> >+unsigned short
> >+nsAccUtils::GetAttributeCharacteristics(nsIAtom* aAtom)
> >+{
> >+    for (PRUint32 i; i < nsARIAMap::gWAIUnivAttrMapLength; i++)
> 
> I really doubt this works - not initialized local variable 'i' - that's the
> reason we strongly need mochitests :)

Ugh. Thanks.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

I am having second thoughts about the hash. Will try to catch you on IRC.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco and I chatted over IRC. Current plan is to stick with the array for now. While the hash is O(1), we don't know the constant time cost. For our n, we might be as good or better of with the array, so we'll wait to do a bigger profiling sweep later before changing at this point. Premature optimization doesn't always work out ;)

There is some test coverage in test_aria_token_attrs.html so I will add a test_aria_object_attrs.html and possibly tweak the former.

Not sure why the uninitialized i in my loop didn't bork anything... perhaps I lucked out (debug builds often add zero'ed padding).

Comment 21

[Aaron Leventhal]

Gee, that sounds familiar. I thought I'd said that.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #20)
> There is some test coverage in test_aria_token_attrs.html so I will add a
> test_aria_object_attrs.html and possibly tweak the former.

After examining test_cssattrs and testgroupattrs, I think I understand the kind
of test Surkov is requesting. I'll see what I can whip up. (Sorry for noise)

(In reply to comment #21)
You probably did at some point.

Comment 23

[Aaron Leventhal]

It's really silly to try to optimize this. How many nodes are there that even
have a role attribute at all? A binary search is definitely fast enough.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: no test yet (see comment)

Surkov, I started writing tests (several times) but had trouble thinking of something that wasn't already covered. I believe I've addressed your other comments.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> It's really silly to try to optimize this. How many nodes are there that even
> have a role attribute at all? A binary search is definitely fast enough.

I didn't mean to optimize this. I'm fine with array for this fix. I just said if hash is really quick then we could keep the information about ARIA attribute type in one place (see comment #17).

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 360313 [details] [diff] [review] [details])
> > I realize this comment is a bit late because you have this patch but. If search
> > in hash is really O(1) then we could introduce hash here instead of array.
> 
> Hash should always be O(1). I was leaning toward a hash, but I chose an array
> after discussing with devs on #developers. We would need a larger n for a hash
> to be worthwhile. I think we can always do that later if we want to, maybe
> after we do some profiling.
> 
> > Advantages are:
> > 1) we hold information about attribute type in unique place
> > 2) we can get rid calls like 
> > nsAccUtils::HasDefinedARIAToken(ancestor, nsAccessibilityAtoms::aria_relevant)
> > &&
> > ancestor->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::aria_relevant,
> > relevant))
> > 
> > you introduced in bug 452388 if we will have GetARIAAttr method.
> 
> Sure, but that sounds like a separate bug?

I'm fine with this. If we want this let's deal in another bug. (Don't forget to file it :))

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> > >+nsAttributeCharacteristics nsARIAMap::gWAIUnivAttrMap[] = {
> > >+  {&nsAccessibilityAtoms::aria_activedescendant,  ATTR_EXPOSEOBJ                 },
> > >+  {&nsAccessibilityAtoms::aria_atomic,                             ATTR_VALTOKEN },
> > 
> > ARIA atoms with characteristics == ATTR_VALTOKEN (note, not characteristics ==
> > ATTR_VALTOKEN | ATTR_VALTOKEN) aren't used in this patch. How do you want to
> > use them? I realize it's possibly bug 473164 but I want to be sure and like to
> > know usecase.
> 
> Can you rephrase? In bug 473164 will check only ATTR_VALTOKEN attributes so
> that we don't expose them if they have value "undefined" -- as per spec.

Ok. If pure ATTR_VALTOKEN values will be used in bug 473164 then I'm fine with this. I guess I will get the usecase of this when I see the patch for that bug. 

> > >+  {&nsAccessibilityAtoms::aria_expanded,                           ATTR_VALTOKEN },
> > 
> > expanded is not object attribute any more
> 
> But it does take token values (bool) right?

Right. But that's not point I meant. aria_expanded is used in IsARIAPropForObjectAttr. But you dropped it from object attributes.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> Not sure why the uninitialized i in my loop didn't bork anything... perhaps I
> lucked out (debug builds often add zero'ed padding).

I can't believe you was so lucky :) Uninitialized local variable has random value (the value that was in memory local variable points to).

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Created an attachment (id=360541) [details]
> no test yet (see comment)
> 
> Surkov, I started writing tests (several times) but had trouble thinking of
> something that wasn't already covered. I believe I've addressed your other
> comments.

Thanks. Don't afraid to cover that was covered once already :). If we don't catch this with Marco then any way it won't be bad.

Here we should tests accessible object attributes (you got this right), iirc we don't have tests object attributes created from ARIA markup.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 360541 [details] [diff] [review]
no test yet (see comment)


>+  {&nsAccessibilityAtoms::aria_expanded,                           ATTR_VALTOKEN },

please get back rights for aria-expanded :)

>+/**
>+ * This mask indicates the attribute is expected to have an NMTOKEN or bool value.
>+ * (See nsAccessible::GetAttributes)

nit: since it's not unique place where ATTR_VALTOKEN will be used place "see, for example"

> 
>+PRUint8
>+nsAccUtils::GetAttributeCharacteristics(nsIAtom* aAtom)
>+{
>+    for (PRUint32 i=0; i < nsARIAMap::gWAIUnivAttrMapLength; i++)

nit: wrap '=' by spaces.

>   /**
>+   * Get the ARIA attribute characteristics for a given aria attr.

nit: "aria" -> "ARIA", "attr" -> "attribute, put new line before this comment and @param lines.

>+   * @param aAtom  A pointer to the nsIAtom representing the aria attr.

nit: just "ARIA attribute"?

>+   * @return       A bitflag representing the attribute characteristics

nit: refere to place where bitflags are defined.

>-      if (!nsAccUtils::IsARIAPropForObjectAttr(attrAtom))
>+      unsigned short attrFlags = nsAccUtils::GetAttributeCharacteristics(attrAtom);

PRUint8

I like to look at the patch with mochitets.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

Addresses comments. Adds tests for attributes that should come through as object attributes according to http://www.w3.org/WAI/PF/aria-implementation/

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 361623 [details] [diff] [review]
patch


>+ * This mask indicates the attribute is expected to have an NMTOKEN or bool value.
>+ * (See for example nsAccessible::GetAttributes)

nit: see for example usage in nsAccessible::GetAttributes

>   /**
>+   * Get the ARIA attribute characteristics for a given ARIA attribute.
>+   * @param aAtom  ARIA attribute

nit: new line between description and @param.

>+   * @return       A bitflag representing the attribute characteristics
>+   *               (see nsARIAMap.h for related bit masks)

nit: "related"? "possible", "expected"? It's worth to mention their names are started from ATTR_ prefix.

>+		test_objectattrs.html \

you forgot to include this file :)

patch looks ok but I need to look at tests.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: this patch includes the test file

(In reply to comment #31)
> (From update of attachment 361623 [details] [diff] [review])
> 
> >+ * This mask indicates the attribute is expected to have an NMTOKEN or bool value.
> >+ * (See for example nsAccessible::GetAttributes)
> 
> nit: see for example usage in nsAccessible::GetAttributes

Done.

> 
> >   /**
> >+   * Get the ARIA attribute characteristics for a given ARIA attribute.
> >+   * @param aAtom  ARIA attribute
> 
> nit: new line between description and @param.

Done.

> 
> >+   * @return       A bitflag representing the attribute characteristics
> >+   *               (see nsARIAMap.h for related bit masks)
> 
> nit: "related"? "possible", "expected"? It's worth to mention their names are
> started from ATTR_ prefix.

Done. Note: normally my personal coding style is not to have code refer to other code so directly as it is hard to maintain.

> 
> >+		test_objectattrs.html \
> 
> you forgot to include this file :)

Done :)

> 
> patch looks ok but I need to look at tests.

I test the properties that are truly exposed as object attributes, and not otherwise captured some other way (like a relation). For exposing "checkable" I spun off bug 477876.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Surkov, one other thing. There might be another spin off bug here but I'll try to catch you on IRC about that. It is to do with the implementation guide and how we are using the concept of "object attribute" in our code.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> I test the properties that are truly exposed as object attributes, and not
> otherwise captured some other way (like a relation). For exposing "checkable" I
> spun off bug 477876.

Why don't we want to test all of them? Also, would be nice to test we don't expose undefined nmtoken attributes.

(In reply to comment #33)
> Surkov, one other thing. There might be another spin off bug here but I'll try
> to catch you on IRC about that. It is to do with the implementation guide and
> how we are using the concept of "object attribute" in our code.

I'll try to be caught on irc tonight :). But any way email or bug is also good in out case.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 361761 [details] [diff] [review]
this patch includes the test file

r=me

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Great, I'll follow up with some additional tests on bug 473164.

Comment 37

[Marco Zehe (:MarcoZ)]

Comment on attachment 361761 [details] [diff] [review]
this patch includes the test file

>+  <div id="activedescendant" role="tablist" aria-activedescendant="tab1">
>+    <span role="tab" id="tab1">tab1</span>
>+    <span role="tab" id="tab2">tab2</span>
>+  </div>

Are you planning to use this in the future? You don't test this one at all yet.

r=me with the above explained.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch to go in.

Marco, nice catch, that was some test content kruft that might not be used in the future. I've removed it. This patch should be good to push.

Thanks guys.

Comment 39

[Marco Zehe (:MarcoZ)]

Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/952b65ce3233

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco if we request 1.9.1. on this, we might want to push fix on bug 482563 along with it... although probably unecessary since it is just about code cosmetics.

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 362050 [details] [diff] [review]
patch to go in.

a191=beltzner

Comment 42

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95dc3abc972e

Comment 43

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)