34612 2010-02-04 13:17:14 -0800 MSAA: accSelect returns error codes for most elements that arent listbox or menupopup related 2010-02-04 15:34:39 -0800 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC Windows XP RESOLVED FIXED InRadar P2 Normal --- 1 alice.barraclough alice.barraclough webkit.review.bot oldest_to_newest 187910 0 alice.barraclough 2010-02-04 13:17:14 -0800 Webkit on Windows implementation of IAccessible::accSelect() returns error codes for most elements that aren't listbox or menupopup related elements. For example, using Microsoft's AccExplorer 2.0 tool on an anchor element in TOT returns E_INVALIDARG for all of the accSelect_* MSAA verification tests. (In contrast, firefox returns S_OK for all the cases that make sense, most of which do.) <rdar://problem/7554699> 187914 1 48162 alice.barraclough 2010-02-04 13:27:02 -0800 Created attachment 48162 patch 187919 2 webkit.review.bot 2010-02-04 13:32:05 -0800 Attachment 48162 did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/win/AccessibleBase.cpp:393: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/win/AccessibleBase.cpp:394: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/win/AccessibleBase.cpp:422: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/win/AccessibleBase.cpp:423: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKit/win/AccessibleBase.cpp:424: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 5 If any of these errors are false positives, please file a bug against check-webkit-style. 187946 3 48162 jhoneycutt 2010-02-04 14:35:51 -0800 Comment on attachment 48162 patch > 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(selectedChildren); > - } 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! 187947 4 48162 jhoneycutt 2010-02-04 14:42:57 -0800 Comment on attachment 48162 patch > 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))) This is a copy/paste error. It should read: ((selectionFlags & (SELFLAG_EXTENDSELECTION | SELFLAG_TAKESELECTION)) == (SELFLAG_EXTENDSELECTION | SELFLAG_TAKESELECTION))) Feel free to fix that :) 187955 5 alice.barraclough 2010-02-04 15:11:49 -0800 (In reply to comment #3) > (From update of attachment 48162 [details]) > > 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. Ya, this is the way it is in Firefox. However, after checking with IE8, i'm deciding to remove this chunk of code because this isn't what IE8 does. 187967 6 alice.barraclough 2010-02-04 15:34:39 -0800 thanks Jon for reivew. I've removed the firefox-specific section, as mentioned in a previous comment. I also fixed what the style bot mentioned, added the assert that jon wanted, and fixed the copy-paste issue. committed r54376 48162 2010-02-04 13:27:02 -0800 2010-02-04 14:42:57 -0800 patch patch-7554699-34612.txt text/plain 3226 alice.barraclough SW5kZXg6IFdlYktpdC93aW4vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC93aW4vQ2hh bmdlTG9nCShyZXZpc2lvbiA1NDM2NSkKKysrIFdlYktpdC93aW4vQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTAtMDItMDQgIEFsaWNlIExpdSAgPGFsaWNlLmxp dUBhcHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM0NjEyICIgTVNB QTogYWNjU2VsZWN0IHJldHVybnMgZXJyb3IgCisgICAgICAgIGNvZGVzIGZvciBtb3N0IGVsZW1l bnRzIHRoYXQgYXJlbnQgbGlzdGJveCBvciBtZW51cG9wdXAgcmVsYXRlZCIKKyAgICAgICAgPHJk YXI6Ly9wcm9ibGVtLzc1NTQ2OTk+CisKKyAgICAgICAgKiBBY2Nlc3NpYmxlQmFzZS5jcHA6Cisg ICAgICAgIChBY2Nlc3NpYmxlQmFzZTo6YWNjU2VsZWN0KToKKyAgICAgICAgLSBTdG9wIHNlbmRp bmcgRV9JTlZBTElEQVJHIGZvciBlbGVtZW50cyB0aGF0IHJlcXVlc3QgVEFLRV9TRUxFQ1RJT04g dGhhdCAKKyAgICAgICAgICBhcmVuJ3QgYmVuZWF0aCBsaXN0Ym94ZXMgb3IgbWVudXBvcHVwcy4g IFRoaXMgd2FzIHRvbyByZXN0cmljdGl2ZSBzaW5jZSAKKyAgICAgICAgICBhbnkgZWxlbWVudCBj YW4gYmUgc2VsZWN0YWJsZS4KKyAgICAgICAgLSBSZXR1cm4gRV9GQUlMIGZvciBhIGZldyBjb21i aW5hdGlvbiBvZiBmbGFncyB0aGF0IGRvbid0IG1ha2Ugc2Vuc2UsIG1hdGNoaW5nIGZpcmVmb3gK KyAgICAgICAgLSBDb3JyZWN0IHRoZSBtaXNpbnRlcnByZXRhdGlvbiBvZiBNU0ROJ3Mgc3RpcHVs YXRpb24gb2Ygc2l0dWF0aW9ucyBpbnZvbHZpbmcgCisgICAgICAgICAgYWRkaW5nLCByZW1vdmlu ZywgYW5kIGV4dGVuZGluZyBzZWxlY3Rpb24gb24gc2luZ2xlLXNlbGVjdCBlbGVtZW50cworCiAy MDEwLTAyLTA0ICBCcmVudCBGdWxnaGFtICA8YmZ1bGdoYW1Ad2Via2l0Lm9yZz4KIAogICAgICAg ICBSZXZpZXdlZCBieSBBZGFtIFJvYmVuLgpJbmRleDogV2ViS2l0L3dpbi9BY2Nlc3NpYmxlQmFz ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L3dpbi9BY2Nlc3NpYmxlQmFzZS5jcHAJKHJldmlz aW9uIDU0Mjk1KQorKysgV2ViS2l0L3dpbi9BY2Nlc3NpYmxlQmFzZS5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTM4OCw2ICszODgsMTMgQEAgSFJFU1VMVCBTVERNRVRIT0RDQUxMVFlQRSBBY2Nlc3Np YmxlQmFzZQogICAgICAgICAoKHNlbGVjdGlvbkZsYWdzICYgKFNFTEZMQUdfRVhURU5EU0VMRUNU SU9OIHwgU0VMRkxBR19UQUtFU0VMRUNUSU9OKSkgPT0gKFNFTEZMQUdfUkVNT1ZFU0VMRUNUSU9O IHwgU0VMRkxBR19UQUtFU0VMRUNUSU9OKSkpCiAgICAgICAgIHJldHVybiBFX0lOVkFMSURBUkc7 CiAKKyAgICAvLyBNYXRjaCBGaXJlZm94IGJ5IHJldHVybmluZyBFX0ZBSUwgZm9yIHRoZXNlIHNp dHVhdGlvbnMgdGhhdCBkb24ndCBtYWtlIHNlbnNlLgorICAgIGNvbnN0IGxvbmcgYWxsU0VMRkxB R3MgPSBTRUxGTEFHX1RBS0VGT0NVUyB8IFNFTEZMQUdfVEFLRVNFTEVDVElPTiB8IFNFTEZMQUdf RVhURU5EU0VMRUNUSU9OIHwgU0VMRkxBR19BRERTRUxFQ1RJT04gfCBTRUxGTEFHX1JFTU9WRVNF TEVDVElPTjsKKyAgICBpZiAoKChzZWxlY3Rpb25GbGFncyAmIGFsbFNFTEZMQUdzKSA9PSBTRUxG TEFHX05PTkUpIHx8CisgICAgICAgICgoc2VsZWN0aW9uRmxhZ3MgJiBhbGxTRUxGTEFHcykgPT0g U0VMRkxBR19BRERTRUxFQ1RJT04pIHx8CisgICAgICAgICgoc2VsZWN0aW9uRmxhZ3MgJiBhbGxT RUxGTEFHcykgPT0gU0VMRkxBR19FWFRFTkRTRUxFQ1RJT04pKQorICAgICAgICByZXR1cm4gRV9G QUlMOworCiAgICAgQWNjZXNzaWJpbGl0eU9iamVjdCogY2hpbGRPYmplY3Q7CiAgICAgSFJFU1VM VCBociA9IGdldEFjY2Vzc2liaWxpdHlPYmplY3RGb3JDaGlsZCh2Q2hpbGQsIGNoaWxkT2JqZWN0 KTsKIApAQCAtNDA2LDE1ICs0MTMsMTYgQEAgSFJFU1VMVCBTVERNRVRIT0RDQUxMVFlQRSBBY2Nl c3NpYmxlQmFzZQogICAgICAgICAgICAgVmVjdG9yPFJlZlB0cjxBY2Nlc3NpYmlsaXR5T2JqZWN0 PiA+IHNlbGVjdGVkQ2hpbGRyZW4oMSk7CiAgICAgICAgICAgICBzZWxlY3RlZENoaWxkcmVuWzBd ID0gY2hpbGRPYmplY3Q7CiAgICAgICAgICAgICBzdGF0aWNfY2FzdDxBY2Nlc3NpYmlsaXR5TGlz dEJveCo+KHBhcmVudE9iamVjdCktPnNldFNlbGVjdGVkQ2hpbGRyZW4oc2VsZWN0ZWRDaGlsZHJl bik7Ci0gICAgICAgIH0gZWxzZSBpZiAocGFyZW50T2JqZWN0LT5pc01lbnVMaXN0UG9wdXAoKSkK KyAgICAgICAgfSBlbHNlIC8vIGFueSBlbGVtZW50IG1heSBiZSBzZWxlY3RhYmxlIGJ5IHZpcnR1 ZSBvZiBpdCBoYXZpbmcgdGhlIGFyaWEtc2VsZWN0ZWQgcHJvcGVydHkKICAgICAgICAgICAgIGNo aWxkT2JqZWN0LT5zZXRTZWxlY3RlZCh0cnVlKTsKLSAgICAgICAgZWxzZQotICAgICAgICAgICAg cmV0dXJuIEVfSU5WQUxJREFSRzsKICAgICB9CiAKLSAgICAvLyBNU0ROIHNheXMgdGhhdCBBREQs IFJFTU9WRSwgYW5kIEVYVEVORFNFTEVDVElPTiBhcmUgaW52YWxpZCBmb3IKKyAgICAvLyBNU0RO IHNheXMgdGhhdCBBREQsIFJFTU9WRSwgYW5kIEVYVEVORFNFTEVDVElPTiB3aXRoIG5vIG90aGVy IGZsYWdzIGFyZSBpbnZhbGlkIGZvcgogICAgIC8vIHNpbmdsZS1zZWxlY3QuCi0gICAgaWYgKCFw YXJlbnRPYmplY3QtPmlzTXVsdGlTZWxlY3RhYmxlKCkpCisgICAgaWYgKCFwYXJlbnRPYmplY3Qt PmlzTXVsdGlTZWxlY3RhYmxlKCkgJiYKKyAgICAgICAgKCgoc2VsZWN0aW9uRmxhZ3MgJiBhbGxT RUxGTEFHcykgPT0gU0VMRkxBR19BRERTRUxFQ1RJT04pIHx8CisgICAgICAgICAoKHNlbGVjdGlv bkZsYWdzICYgYWxsU0VMRkxBR3MpID09IFNFTEZMQUdfUkVNT1ZFU0VMRUNUSU9OKSB8fAorICAg ICAgICAgKChzZWxlY3Rpb25GbGFncyAmIGFsbFNFTEZMQUdzKSA9PSBTRUxGTEFHX0VYVEVORFNF TEVDVElPTikpKQogICAgICAgICByZXR1cm4gRV9JTlZBTElEQVJHOwogCiAgICAgaWYgKHNlbGVj dGlvbkZsYWdzICYgU0VMRkxBR19BRERTRUxFQ1RJT04pCg==