Bug 33773 – MSAA: The child <option> elements of a non-multiple <select> are not exposed

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Jon Honeycutt]

The accessibility of the child <option> elements of a non-multiple <select> element do not appear in the accessibility tree.

[Jon Honeycutt]

<rdar://problem/7550556>

[Jon Honeycutt]

Created attachment 46850 [details]
patch

[Alice Liu]

Comment on attachment 46850 [details]
patch


> diff --git a/WebCore/WebCore.vcproj/WebCore.vcproj b/WebCore/WebCore.vcproj/WebCore.vcproj
> index bc40848..d27492c 100644
> --- a/WebCore/WebCore.vcproj/WebCore.vcproj
> +++ b/WebCore/WebCore.vcproj/WebCore.vcproj
> @@ -20380,14 +20404,6 @@
>  				RelativePath="..\accessibility\AXObjectCache.cpp"
>  				>
>  				<FileConfiguration
> -					Name="Debug|Win32"
> -					ExcludedFromBuild="true"
> -					>
> -					<Tool
> -						Name="VCCLCompilerTool"
> -					/>
> -				</FileConfiguration>
> -				<FileConfiguration
>  					Name="Release|Win32"
>  					ExcludedFromBuild="true"
>  					>

i guess you'll want to put this back.

> diff --git a/WebCore/accessibility/AccessibilityMenuListHiddenList.cpp b/WebCore/accessibility/AccessibilityMenuListHiddenList.cpp
> +void AccessibilityMenuListHiddenList::addChildren()
> +{
> +    Node* selectNode = m_menuList->renderer()->node();
> +    if (!selectNode)
> +        return;
> +
> +    m_haveChildren = true;
> +
> +    const Vector<Element*>& listItems = static_cast<HTMLSelectElement*>(selectNode)->listItems();

All your other code seems to have checks to ensure the safety of the static cast, but it's not present here. 


> diff --git a/WebCore/accessibility/AccessibilityMenuListOption.cpp b/WebCore/accessibility/AccessibilityMenuListOption.cpp
> +bool AccessibilityMenuListOption::isVisible() const
> +{
> +    // In a single-option select, only the selected item is considered visible.
> +    return isSelected();
> +}
> +

This doesn't seem right if a menu is expanded.  please investigate.  If you can't find out a definitive answer by using an inspection tool on firefox, then i would suggest adding a case for isExpanded().  


> diff --git a/WebCore/accessibility/AccessibilityMenuListOption.h b/WebCore/accessibility/AccessibilityMenuListOption.h

> +    void setElement(HTMLElement* element);

I forgot to mention this earlier, but i think our style is to leave out the name of the param in unambiguous circumstances.  applies to rest of all your files. 


r- only for the suspected incorrectness of AccessibilityMenuListOption::isVisible()

[Jon Honeycutt]

(In reply to comment #3)
> (From update of attachment 46850 [details])
> 
> > diff --git a/WebCore/WebCore.vcproj/WebCore.vcproj b/WebCore/WebCore.vcproj/WebCore.vcproj
> > index bc40848..d27492c 100644
> > --- a/WebCore/WebCore.vcproj/WebCore.vcproj
> > +++ b/WebCore/WebCore.vcproj/WebCore.vcproj
> > @@ -20380,14 +20404,6 @@
> >  				RelativePath="..\accessibility\AXObjectCache.cpp"
> >  				>
> >  				<FileConfiguration
> > -					Name="Debug|Win32"
> > -					ExcludedFromBuild="true"
> > -					>
> > -					<Tool
> > -						Name="VCCLCompilerTool"
> > -					/>
> > -				</FileConfiguration>
> > -				<FileConfiguration
> >  					Name="Release|Win32"
> >  					ExcludedFromBuild="true"
> >  					>
> 
> i guess you'll want to put this back.

Fixed.

> 
> > diff --git a/WebCore/accessibility/AccessibilityMenuListHiddenList.cpp b/WebCore/accessibility/AccessibilityMenuListHiddenList.cpp
> > +void AccessibilityMenuListHiddenList::addChildren()
> > +{
> > +    Node* selectNode = m_menuList->renderer()->node();
> > +    if (!selectNode)
> > +        return;
> > +
> > +    m_haveChildren = true;
> > +
> > +    const Vector<Element*>& listItems = static_cast<HTMLSelectElement*>(selectNode)->listItems();
> 
> All your other code seems to have checks to ensure the safety of the static
> cast, but it's not present here. 

I added an ASSERT that the node has the expected tag name.

> 
> 
> > diff --git a/WebCore/accessibility/AccessibilityMenuListOption.cpp b/WebCore/accessibility/AccessibilityMenuListOption.cpp
> > +bool AccessibilityMenuListOption::isVisible() const
> > +{
> > +    // In a single-option select, only the selected item is considered visible.
> > +    return isSelected();
> > +}
> > +
> 
> This doesn't seem right if a menu is expanded.  please investigate.  If you
> can't find out a definitive answer by using an inspection tool on firefox, then
> i would suggest adding a case for isExpanded().  

You're right - fixed.

> 
> 
> > diff --git a/WebCore/accessibility/AccessibilityMenuListOption.h b/WebCore/accessibility/AccessibilityMenuListOption.h
> 
> > +    void setElement(HTMLElement* element);
> 
> I forgot to mention this earlier, but i think our style is to leave out the
> name of the param in unambiguous circumstances.  applies to rest of all your
> files. 

Fixed.

> 
> 
> r- only for the suspected incorrectness of
> AccessibilityMenuListOption::isVisible()

Thanks for the review! Next patch coming shortly.

[Jon Honeycutt]

Created attachment 46947 [details]
patch

This patch adds Alice's suggested changes.

The class AccessibilityMenuListHiddenList was renamed to AccessibilityMenuListPopup to better reflect which object in the tree it represents.

The "collapsed", "expanded", and "has popup" states were added to the MSAA wrapper object.

To test the expanded state of the select menu, we now allow the menu to be expanded and collapsed via the MSAA wrapper's accDoDefaultAction() call. Code was added to the layout test to test the popup menu's state and the state of its child option elements with the menu expanded.

[Alice Liu]

awesome.  r+

[Jon Honeycutt]

Committed in r53512. Added some pre-emptive build fixes by stubbing out the localized string functions on other platforms.

[Eric Seidel (no email)]

I think this broke all 3 chromium builds.

The chromium EWS should have caught this, but looks like this wasn't up for review quite long enough for it to finish.  We'll need to make it faster.  https://bugs.webkit.org/show_activity.cgi?id=33773

[Eric Seidel (no email)]

I'm not sure this actually was the cause of the chromium break yet:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/1210/steps/compile-webkit/logs/stdio