Bug 37040 - AX: need to send selected children change notification when aria-selected changed
Summary: AX: need to send selected children change notification when aria-selected cha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-02 13:03 PDT by chris fleizach
Modified: 2010-04-05 15:15 PDT (History)
4 users (show)

See Also:


Attachments
patch (11.31 KB, patch)
2010-04-05 12:10 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.