468034 - make automated tests for name calculation rules

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

these automated tests should replace hard-to-read/hard-to-write-and-easy-to-mistake mochitests for name calculation rules for DOM elements (like test_nsIAccessible_name_button.html and test_nsIAccessible_name_link.html).

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

for you judgment

Comment 2

[Marco Zehe (:MarcoZ)]

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

It'll take me a moment to digest what you're doing here. I think I understand most of it, but need a bit more time to digest the whole thing. One thing I noticed:

>diff --git a/accessible/tests/mochitest/namerules.xml b/accessible/tests/mochitest/namerules.xml

This file is missing from the Makefine.in.

Comment 3

[Marco Zehe (:MarcoZ)]

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

>+function htmlDocResolver(aPrefix) {

Nit, please move { onto its own line.

Comment 4

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

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

> /**
>- * Return accessible for the given ID attribute or DOM element or accessible.
>+ * Return accessible for the given identificator (may be ID attribute or DOM
>+ * element or accessible object).

For now, please change "identificator" to "identifier" in this function header (3 places). I'll look at the rest after my current distraction.

Comment 5

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

Surkov, I'll try to catch you on IRC, I think the patch is good just want to understand reasons for the approach.

Comment 6

[Marco Zehe (:MarcoZ)]

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

After chatting on IRC about this earlier today, I can now understand what this patch actually does. It gives us a means of testing various scenarios in a very efficient manner. For example, first test what the accName is if aria-label is present, then remove that aria-label, and test again to make sure now aria-labelledby is used. While this is not exactly a real-life use case, it gives us the capability to test varying markup on the same element.

I'm giving this an r+, but request that you add comments for the techniques the functions use, and ask me to re-review those.

Thanks!

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 357121 [details] [diff] [review]
patch2

1. As David already requested, replace "identificator" with "identifier".
2.
>+     'aria-label' attribute. In this case 'rule' element has 'attr' attribute
>+     pointing to attribute name and 'type' attribute with 'sting' value. For

Nit: Missing "r" in "string value".

3.
>+   * name is calculated from elements that are pointed by attribute value on

"pointed to by".

>+     the element. Example is 'aria-labelledby'. In this case 'rule' element
>+     has 'attr' attribute holding the sequence of IDs of elements used to
>+     compute the name. As well 'rule' element has 'type' attribute with 'ref'

Replace "As well rule element ..." with "in addition the rule element..."

4.
>+  * name is computed from subtree. Example, html:button. In this case 'rule'
>+    element has 'fromsbutree' attribute with 'true' value. For example,

Typo in attribute name "fromsubtree", b and u mixed up.

5.
>+  (let's call it test element). As well 'markup' element contains some other

Again replace "as well" with "In addition the".

>+  Test element is pointed by 'ref' attribute on 'markup' element. Also 'markup'

"pointed to by" please.

>+  element has 'ruleset' attribute to point ruleset for the test element.

This should be "to indicate ruleset..."


6.
>+  Initially 'markup' element hold markup for all rules specified by 'ruleset'

"holds".

>+  attribute. This allows to check us if the sequence of name computation rules

"this allows us to check if ...".

>+  is correct. Here 'ruleset' element defines two rules. We get the first rule
>+  which means accesisble name is computed from value of 'aria-label' attribute.

Typo: Accessible" instead of "accessisble".

>+  Then we check accessible name for the test element and remove 'aria-label'
>+  attribute. After we get the second rule which means we should get IDs from
>+  'aria-labelledby' attribute and compose accessible name from values of
>+  a11yname' attributes placed on elements with obtained IDs. Check accessible
>+  name and finish test.

You need to explicitly specify what the 'a11yname' attribute is supposed to do. It is supposed to give the desired name for each element that is being pointed to by aria-labelledby. Would be good if you could insert a paragraph above the example explaining that.

Thanks!

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (From update of attachment 357121 [details] [diff] [review])
> 1. As David already requested, replace "identificator" with "identifier".

I replaced it at my local copy.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 357125 [details] [diff] [review]
patch3

This is good, thanks!

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 357125 [details] [diff] [review]
patch3

I forgot to requery review from you.

Comment 13

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

Comment on attachment 357125 [details] [diff] [review]
patch3

Surkov, your comments helped a lot thanks! r+

Comment 14

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/da2def306eda