Closed Bug 468034 Opened 16 years ago Closed 16 years ago

make automated tests for name calculation rules

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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).
OS: Mac OS X → All
Hardware: PC → All
Attached patch patch (obsolete) (deleted) — Splinter Review
for you judgment
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #356483 - Flags: review?(marco.zehe)
Attachment #356483 - Flags: review?(david.bolter)
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 on attachment 356483 [details] [diff] [review] patch >+function htmlDocResolver(aPrefix) { Nit, please move { onto its own line.
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.
Surkov, I'll try to catch you on IRC, I think the patch is good just want to understand reasons for the approach.
Attachment #356483 - Flags: review?(marco.zehe) → review+
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!
Attached patch patch2 (obsolete) (deleted) — Splinter Review
Attachment #356483 - Attachment is obsolete: true
Attachment #357121 - Flags: review?(marco.zehe)
Attachment #356483 - Flags: review?(david.bolter)
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!
Attached patch patch3 (deleted) — Splinter Review
Attachment #357121 - Attachment is obsolete: true
Attachment #357125 - Flags: review?(marco.zehe)
Attachment #357121 - Flags: review?(marco.zehe)
(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 on attachment 357125 [details] [diff] [review] patch3 This is good, thanks!
Attachment #357125 - Flags: review?(marco.zehe) → review+
Attachment #357125 - Flags: review?(david.bolter)
Comment on attachment 357125 [details] [diff] [review] patch3 I forgot to requery review from you.
Comment on attachment 357125 [details] [diff] [review] patch3 Surkov, your comments helped a lot thanks! r+
Attachment #357125 - Flags: review?(david.bolter) → review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: