Extend nsARIAMap or create hashtable to capture ARIA attribute characteristics

RESOLVED FIXED in mozilla1.9.1b4

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

({access, verified1.9.1})

unspecified
mozilla1.9.1b4
access, verified1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

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.
Summary: Extend nsARIAMap to include ARIA attribute characteristics → Extend nsARIAMap or create hashtable to capture ARIA attribute characteristics
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 3

9 years ago
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?
Created attachment 359110 [details] [diff] [review]
WIP2 - shows code we can remove.
Attachment #359060 - Attachment is obsolete: true
Attachment #359110 - Attachment is patch: true
Attachment #359110 - Attachment mime type: application/octet-stream → text/plain
Note this also removes the need to add code from on patch bug 473164 (i.e. I fix that bug here too)

Comment 6

9 years ago
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.
(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.
Attachment #359110 - Flags: review?(surkov.alexander)
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.

Updated

9 years ago
Assignee: david.bolter → nobody
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis

Comment 9

9 years ago
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.

Updated

9 years ago
Attachment #359110 - Flags: review?(surkov.alexander)

Comment 10

9 years ago
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?
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED
Created attachment 359557 [details] [diff] [review]
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)
Attachment #359110 - Attachment is obsolete: true
Attachment #359557 - Flags: review?(surkov.alexander)

Comment 12

9 years ago
(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
Created attachment 360084 [details] [diff] [review]
WIP - for discussion
Attachment #359557 - Attachment is obsolete: true
Attachment #360084 - Flags: review?(surkov.alexander)
Attachment #359557 - Flags: review?(surkov.alexander)

Updated

9 years ago
Attachment #360084 - Attachment is patch: true
Attachment #360084 - Attachment mime type: application/octet-stream → text/plain
Created attachment 360121 [details] [diff] [review]
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.
Attachment #360084 - Attachment is obsolete: true
Attachment #360121 - Flags: review?(surkov.alexander)
Attachment #360084 - Flags: review?(surkov.alexander)

Comment 15

9 years ago
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?
Created attachment 360313 [details] [diff] [review]
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.
Attachment #360121 - Attachment is obsolete: true
Attachment #360313 - Flags: review?(surkov.alexander)
Attachment #360121 - Flags: review?(surkov.alexander)

Comment 17

9 years ago
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 :)
Attachment #360313 - Flags: review?(surkov.alexander)
(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.
I am having second thoughts about the hash. Will try to catch you on IRC.
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

9 years ago
Gee, that sounds familiar. I thought I'd said that.
(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

9 years ago
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.
Created attachment 360541 [details] [diff] [review]
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.
Attachment #360313 - Attachment is obsolete: true
Attachment #360541 - Flags: review?(surkov.alexander)

Comment 25

9 years ago
(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

9 years ago
(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

9 years ago
(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

9 years ago
(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

9 years ago
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.
Attachment #360541 - Flags: review?(surkov.alexander)
Created attachment 361623 [details] [diff] [review]
patch

Addresses comments. Adds tests for attributes that should come through as object attributes according to http://www.w3.org/WAI/PF/aria-implementation/
Attachment #360541 - Attachment is obsolete: true
Attachment #361623 - Flags: review?(surkov.alexander)

Comment 31

9 years ago
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.
Attachment #361623 - Flags: review?(surkov.alexander)
Created attachment 361761 [details] [diff] [review]
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.
Attachment #361623 - Attachment is obsolete: true
Attachment #361761 - Flags: review?(surkov.alexander)
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

9 years ago
(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.

Updated

9 years ago
Attachment #361761 - Flags: review?(surkov.alexander) → review+

Comment 35

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

r=me
Great, I'll follow up with some additional tests on bug 473164.
Attachment #361761 - Flags: review?(marco.zehe)

Updated

9 years ago
Attachment #361761 - Flags: review?(marco.zehe) → review+
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.
Created attachment 362050 [details] [diff] [review]
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.
Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/952b65ce3233
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Attachment #362050 - Flags: approval1.9.1?
Attachment #362050 - Flags: approval1.9.1?
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.
Flags: wanted1.9.1?

Updated

9 years ago
Attachment #362050 - Flags: approval1.9.1?

Updated

9 years ago
Flags: wanted1.9.1?
Comment on attachment 362050 [details] [diff] [review]
patch to go in.

a191=beltzner
Attachment #362050 - Flags: approval1.9.1? → approval1.9.1+
Pushed to mozilla-1.9.1 on David's behalf in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95dc3abc972e
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4
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)
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.