Bug 37040

Summary: AX: need to send selected children change notification when aria-selected changed
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bdakin, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch darin: review+

Description chris fleizach 2010-04-02 13:03:59 PDT
when aria-selected changes, webkit should send out the notification
Comment 1 chris fleizach 2010-04-05 12:10:29 PDT
Created attachment 52556 [details]
patch
Comment 2 Darin Adler 2010-04-05 12:18:44 PDT
Comment on attachment 52556 [details]
patch

> +    bool nodeIsAriaType(Node*, String role);

The type should be const String&. If the term for the argument really role, then I'd think you'd want it to be called nodeHasRole rather than nodeIsAriaType. Or nodeHasARIAType.

> -        if (renderer->isTextControl())
> -            return renderer->document()->axObjectCache()->getOrCreate(renderer);
> +        if (renderer->isTextControl() 
> +            || (renderer->isListBox() || axObjectCache()->nodeIsAriaType(renderer->node(), "listbox")))
> +            return axObjectCache()->getOrCreate(renderer);

This seems like a pretty strange set of conditions. I think this rule should be implemented with a helper function, just to give you a place to put a comment. And the comment can explain why these three things are special.

> +    AccessibilityChildrenVector childObjects = children();
> +    for (unsigned k = 0, childrenSize = childObjects.size(); k < childrenSize; ++k) {

Normally we'd just initialize childrenSize outside the loop. It's neat to have it scoped like this, but clearer written the more-conventional way.
Comment 3 chris fleizach 2010-04-05 13:42:12 PDT
made changes darin requested
http://trac.webkit.org/changeset/57093
Comment 4 chris fleizach 2010-04-05 14:05:57 PDT
this caused aria-live region to fail on leopard again. it must be something with that test, because it looks like we can't add new tests. i will look at that test now
Comment 5 chris fleizach 2010-04-05 14:09:07 PDT
why this only fails on leopard release bot is baffling
Comment 6 chris fleizach 2010-04-05 14:12:45 PDT
disabling this test on leopard for now
https://bugs.webkit.org/show_bug.cgi?id=37112
Comment 7 WebKit Review Bot 2010-04-05 15:13:25 PDT
http://trac.webkit.org/changeset/57093 might have broken Tiger Intel Release
Comment 8 Adam Barth 2010-04-05 15:15:02 PDT
> http://trac.webkit.org/changeset/57093 might have broken Tiger Intel Release

This is svg/custom/getsvgdocument.html, which has recently become very flaky.  I don't think it's related to your change.