Bug 33773 - MSAA: The child <option> elements of a non-multiple <select> are not exposed
: MSAA: The child <option> elements of a non-multiple <select> are not exposed
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To: Jon Honeycutt
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-17 14:26 PST by Jon Honeycutt
Modified: 2010-01-19 18:04 PST (History)
4 users (show)

See Also:


Attachments
patch (67.81 KB, patch)
2010-01-18 14:14 PST, Jon Honeycutt
alice.barraclough: review-
jhoneycutt: commit‑queue-
Details | Formatted Diff | Diff
patch (79.99 KB, patch)
2010-01-19 13:38 PST, Jon Honeycutt
alice.barraclough: 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 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 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 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