[webkit-reviews] review granted: [Bug 34612] MSAA: accSelect returns error codes for most elements that arent listbox or menupopup related : [Attachment 48162] patch

[bugzilla-daemon at webkit.org]

Jon Honeycutt <jhoneycutt at apple.com> has granted Alice Liu
<alice.liu at apple.com>'s request for review:
Bug 34612: MSAA: accSelect returns error codes for most elements that arent
listbox or menupopup related
https://bugs.webkit.org/show_bug.cgi?id=34612

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

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
> Index: WebKit/win/AccessibleBase.cpp
> ===================================================================
> --- WebKit/win/AccessibleBase.cpp	(revision 54295)
> +++ WebKit/win/AccessibleBase.cpp	(working copy)
> @@ -388,6 +388,13 @@ HRESULT STDMETHODCALLTYPE AccessibleBase
>	   ((selectionFlags & (SELFLAG_EXTENDSELECTION |
SELFLAG_TAKESELECTION)) == (SELFLAG_REMOVESELECTION | SELFLAG_TAKESELECTION)))
>	   return E_INVALIDARG;
>  
> +    // Match Firefox by returning E_FAIL for these situations that don't
make sense.
> +    const long allSELFLAGs = SELFLAG_TAKEFOCUS | SELFLAG_TAKESELECTION |
SELFLAG_EXTENDSELECTION | SELFLAG_ADDSELECTION | SELFLAG_REMOVESELECTION;
> +    if (((selectionFlags & allSELFLAGs) == SELFLAG_NONE) ||
> +	   ((selectionFlags & allSELFLAGs) == SELFLAG_ADDSELECTION) ||
> +	   ((selectionFlags & allSELFLAGs) == SELFLAG_EXTENDSELECTION))
> +	   return E_FAIL;
> +

Firefox doesn't allow you to add an object to the selection or extend the
selection without also passing TAKEFOCUS? MSDN seems to say that ADDSELECTION
and EXTENDSELECTION are valid without any other flags.

>      AccessibilityObject* childObject;
>      HRESULT hr = getAccessibilityObjectForChild(vChild, childObject);
>  
> @@ -406,15 +413,16 @@ HRESULT STDMETHODCALLTYPE AccessibleBase
>	       Vector<RefPtr<AccessibilityObject> > selectedChildren(1);
>	       selectedChildren[0] = childObject;
>	      
static_cast<AccessibilityListBox*>(parentObject)->setSelectedChildren(selectedC
hildren);
> -	   } else if (parentObject->isMenuListPopup())
> +	   } else // any element may be selectable by virtue of it having the
aria-selected property
>	       childObject->setSelected(true);
> -	   else
> -	       return E_INVALIDARG;
>      }

TAKESELECTION is meant to select that object and deselect any other objects, so
it might be good to ASSERT(!parentObject->isMultiSelectable()) in the else
condition to catch cases where TAKESELECTION could allow multiple items to be
selected.

r=me! Thanks for fixing this!