[webkit-reviews] review denied: [Bug 33773] MSAA: The child <option> elements of a non-multiple <select> are not exposed : [Attachment 46850] patch

[bugzilla-daemon at webkit.org]

Alice Liu <alice.liu at apple.com> has denied Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 33773: MSAA: The child <option> elements of a non-multiple <select> are not
exposed
https://bugs.webkit.org/show_bug.cgi?id=33773

Attachment 46850: patch
https://bugs.webkit.org/attachment.cgi?id=46850&action=review

------- Additional Comments from Alice Liu <alice.liu at apple.com>

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