Bug 33773

<rdar://problem/7550556>

Created attachment 46850 [details] patch

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()

(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.

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.

awesome. r+

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

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

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