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
: WebKit
Accessibility
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-01-17 14:26 PST by
Modified: 2010-01-19 18:04 PST (History)


Attachments
patch (67.81 KB, patch)
2010-01-18 14:14 PST, Jon Honeycutt
alice.barraclough: review-
jhoneycutt: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch (79.99 KB, patch)
2010-01-19 13:38 PST, Jon Honeycutt
alice.barraclough: review+
jhoneycutt: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-01-17 14:28:04 PST -------
<rdar://problem/7550556>
------- Comment #2 From 2010-01-18 14:14:28 PST -------
Created an attachment (id=46850) [details]
patch
------- Comment #3 From 2010-01-18 15:39:46 PST -------
(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.

> 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 From 2010-01-19 13:28:53 PST -------
(In reply to comment #3)
> (From update of attachment 46850 [details] [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 From 2010-01-19 13:38:09 PST -------
Created an attachment (id=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 From 2010-01-19 13:50:02 PST -------
awesome.  r+
------- Comment #7 From 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 From 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 From 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