Bug 33088 - MSAA: <select> elements should broadcast value change events
Summary: MSAA: <select> elements should broadcast value change events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-12-31 16:38 PST by Jon Honeycutt
Modified: 2010-01-05 14:14 PST (History)
0 users

See Also:


Attachments
patch (24.18 KB, patch)
2010-01-01 02:43 PST, Jon Honeycutt
darin: review+
jhoneycutt: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 2009-12-31 16:38:35 PST
WebKit should broadcast an EVENT_OBJECT_VALUECHANGE event when the selected index of a non-multiple <select> element changes.

<rdar://problem/7332364>
Comment 1 Jon Honeycutt 2010-01-01 02:43:30 PST
Created attachment 45730 [details]
patch
Comment 2 Darin Adler 2010-01-04 08:20:33 PST
Comment on attachment 45730 [details]
patch

> +    enum PostType { PostSynchronously, PostAsynchronously };
> +    void postNotification(RenderObject*, AXNotification, bool postToElement, PostType = PostAsynchronously);

One the one hand, it's good to not pollute the global WebCore namespace with something like PostType. But on the other, the whole point of such enums is to make code more readable, and if call sites have to utter AXObjectCache::PostSynchronously it's pretty awkward. I suggest moving such enums out of the class.

r=me as is though
Comment 3 Jon Honeycutt 2010-01-04 17:12:40 PST
(In reply to comment #2)
> (From update of attachment 45730 [details])
> > +    enum PostType { PostSynchronously, PostAsynchronously };
> > +    void postNotification(RenderObject*, AXNotification, bool postToElement, PostType = PostAsynchronously);
> 
> One the one hand, it's good to not pollute the global WebCore namespace with
> something like PostType. But on the other, the whole point of such enums is to
> make code more readable, and if call sites have to utter
> AXObjectCache::PostSynchronously it's pretty awkward. I suggest moving such
> enums out of the class.
> 
> r=me as is though

I made this change.

Committed in r52773.
Comment 4 Adam Roben (:aroben) 2010-01-05 06:37:47 PST
Comment on attachment 45730 [details]
patch

> +++ b/WebCore/dom/SelectElement.cpp
> @@ -349,6 +349,8 @@ void SelectElement::setSelectedIndex(SelectElementData& data, Element* element,
>          data.setUserDrivenChange(userDrivenChange);
>          if (fireOnChangeNow)
>              menuListOnChange(data, element);
> +        if (RenderMenuList* menuList = toRenderMenuList(element->renderer()))
> +            menuList->didSetSelectedIndex();
>      }

What if the <select> has no renderer (e.g., it's display:none and someone modifies selectedIndex via JS)? Should we still broadcast in that case?
Comment 5 Jon Honeycutt 2010-01-05 14:14:00 PST
(In reply to comment #4)
> (From update of attachment 45730 [details])
> > +++ b/WebCore/dom/SelectElement.cpp
> > @@ -349,6 +349,8 @@ void SelectElement::setSelectedIndex(SelectElementData& data, Element* element,
> >          data.setUserDrivenChange(userDrivenChange);
> >          if (fireOnChangeNow)
> >              menuListOnChange(data, element);
> > +        if (RenderMenuList* menuList = toRenderMenuList(element->renderer()))
> > +            menuList->didSetSelectedIndex();
> >      }
> 
> What if the <select> has no renderer (e.g., it's display:none and someone
> modifies selectedIndex via JS)? Should we still broadcast in that case?

No, a <select> with no renderer would not be exposed in our accessibility tree.