Bug 33773

Summary: MSAA: The child <option> elements of a non-multiple <select> are not exposed
Product: WebKit Reporter: Jon Honeycutt <jhoneycutt>
Component: AccessibilityAssignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED FIXED    
Severity: Normal CC: alice.barraclough, dglazkov, eric, yaar
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
patch
alice.barraclough: review-, jhoneycutt: commit-queue-
patch alice.barraclough: review+, jhoneycutt: commit-queue-

Description Jon Honeycutt 2010-01-17 14:26:05 PST
The accessibility of the child <option> elements of a non-multiple <select> element do not appear in the accessibility tree.
Comment 1 Jon Honeycutt 2010-01-17 14:28:04 PST
<rdar://problem/7550556>
Comment 2 Jon Honeycutt 2010-01-18 14:14:28 PST
Created attachment 46850 [details]
patch
Comment 3 Alice Liu 2010-01-18 15:39:46 PST
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()
Comment 4 Jon Honeycutt 2010-01-19 13:28:53 PST
(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.
Comment 5 Jon Honeycutt 2010-01-19 13:38:09 PST
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.
Comment 6 Alice Liu 2010-01-19 13:50:02 PST
awesome.  r+
Comment 7 Jon Honeycutt 2010-01-19 17:45:09 PST
Committed in r53512. Added some pre-emptive build fixes by stubbing out the localized string functions on other platforms.
Comment 8 Eric Seidel (no email) 2010-01-19 18:01:08 PST
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
Comment 9 Eric Seidel (no email) 2010-01-19 18:02:35 PST
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