Bug 199216

Summary: Enhance support of aria-haspopup per ARIA 1.1 specification.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, jcraig, jdiggs, rniwa, ryanhaddad, samuel_white, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andres Gonzalez 2019-06-25 18:26:16 PDT
Enhance support of aria-haspopup per ARIA 1.1 specification.
Comment 1 Andres Gonzalez 2019-06-25 18:44:31 PDT
Created attachment 372892 [details]
Patch
Comment 2 chris fleizach 2019-06-25 19:00:54 PDT
Comment on attachment 372892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372892&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:2767
> +    if (equalLettersIgnoringASCIICase(hasPopup, "dialog"))

should we make a static HashSet of valid values and then just check based on passing in the popupAttr.lowercaseString() ?
Comment 3 EWS Watchlist 2019-06-25 19:45:29 PDT
Comment on attachment 372892 [details]
Patch

Attachment 372892 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12576757

New failing tests:
accessibility/button-with-aria-haspopup-role.html
Comment 4 EWS Watchlist 2019-06-25 19:45:31 PDT
Created attachment 372895 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 EWS Watchlist 2019-06-25 20:36:08 PDT
Comment on attachment 372892 [details]
Patch

Attachment 372892 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12576854

New failing tests:
accessibility/button-with-aria-haspopup-role.html
Comment 6 EWS Watchlist 2019-06-25 20:36:10 PDT
Created attachment 372899 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 7 Andres Gonzalez 2019-06-26 08:41:50 PDT
Created attachment 372927 [details]
Patch
Comment 8 Andres Gonzalez 2019-06-26 19:56:50 PDT
Created attachment 372997 [details]
Patch
Comment 9 Andres Gonzalez 2019-06-26 20:05:45 PDT
(In reply to chris fleizach from comment #2)
> Comment on attachment 372892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372892&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2767
> > +    if (equalLettersIgnoringASCIICase(hasPopup, "dialog"))
> 
> should we make a static HashSet of valid values and then just check based on
> passing in the popupAttr.lowercaseString() ?

Thanks for the suggestion. Using HashSet in latest patch. Doesn't buy us much in terms of performance, but code is more elegant and compact. Still have to handle a couple of cases separately. Was looking for an equivalent to boost::hana strings in WTF, but couldn't find it. boost::hana would be ideal for cases like this because the hana strings are as efficient as literals but the set also have the searching capabilities like contains.

Also fixed the DumpRenderTree build and test in latest upload.
Comment 10 chris fleizach 2019-06-26 20:14:08 PDT
Comment on attachment 372997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372997&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:2767
> +    AtomString hasPopup = getAttribute(aria_haspopupAttr).convertToASCIILowercase();

auto popupValue

> Source/WebCore/accessibility/AccessibilityObject.cpp:2774
> +    // aria-haspopup specification states that true must be treated as menu

end sentence with a period

> Tools/ChangeLog:20
> +        Enhance support of aria-haspopup per ARIA 1.1 specification.

remove duplicate change log entry
Comment 11 Andres Gonzalez 2019-06-27 08:21:06 PDT
Created attachment 373032 [details]
Patch
Comment 12 chris fleizach 2019-06-28 08:57:18 PDT
Comment on attachment 373032 [details]
Patch

looks like this win failure is related to the patch

   Creating library C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/DumpRenderTreeLib.lib and object C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/DumpRenderTreeLib.exp
AccessibilityUIElement.obj : error LNK2019: unresolved external symbol "public: class JSRetainPtr<struct OpaqueJSString *> __thiscall AccessibilityUIElement::popupValue(void)const " (?popupValue@AccessibilityUIElement@@QBE?AV?$JSRetainPtr@PAUOpaqueJSString@@@@XZ) referenced in function "struct OpaqueJSValue const * __cdecl getPopupValueCallback(struct OpaqueJSContext const *,struct OpaqueJSValue *,struct OpaqueJSString *,struct OpaqueJSValue const * *)" (?getPopupValueCallback@@YAPBUOpaqueJSValue@@PBUOpaqueJSContext@@PAU1@PAUOpaqueJSString@@PAPBU1@@Z) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.vcxproj]
Comment 13 Andres Gonzalez 2019-06-30 11:07:33 PDT
Created attachment 373193 [details]
Patch
Comment 14 Andres Gonzalez 2019-06-30 11:10:35 PDT
(In reply to chris fleizach from comment #12)
> Comment on attachment 373032 [details]
> Patch
> 
> looks like this win failure is related to the patch
> 
>    Creating library
> C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/DumpRenderTreeLib.
> lib and object
> C:/cygwin/home/buildbot/WebKit/WebKitBuild/Release/lib32/DumpRenderTreeLib.
> exp
> AccessibilityUIElement.obj : error LNK2019: unresolved external symbol
> "public: class JSRetainPtr<struct OpaqueJSString *> __thiscall
> AccessibilityUIElement::popupValue(void)const "
> (?popupValue@AccessibilityUIElement@@QBE?AV?$JSRetainPtr@PAUOpaqueJSString@@@
> @XZ) referenced in function "struct OpaqueJSValue const * __cdecl
> getPopupValueCallback(struct OpaqueJSContext const *,struct OpaqueJSValue
> *,struct OpaqueJSString *,struct OpaqueJSValue const * *)"
> (?getPopupValueCallback@@YAPBUOpaqueJSValue@@PBUOpaqueJSContext@@PAU1@PAUOpaq
> ueJSString@@PAPBU1@@Z)
> [C:
> \cygwin\home\buildbot\WebKit\WebKitBuild\Release\Tools\DumpRenderTree\DumpRen
> derTreeLib.vcxproj]

Fix for DRT Win build in latest patch upload.
Comment 15 chris fleizach 2019-06-30 13:39:41 PDT
wincairo failure now

  Creating library lib64\TestRunnerInjectedBundle.lib and object lib64\TestRunnerInjectedBundle.exp
JSAccessibilityUIElement.cpp.obj : error LNK2019: unresolved external symbol "public: class JSRetainPtr<struct OpaqueJSString *> __cdecl WTR::AccessibilityUIElement::popupValue(void)const " (?popupValue@AccessibilityUIElement@WTR@@QEBA?AV?$JSRetainPtr@PEAUOpaqueJSString@@@@XZ) referenced in function "private: static struct OpaqueJSValue const * __cdecl WTR::JSAccessibilityUIElement::popupValue(struct OpaqueJSContext const *,struct OpaqueJSValue *,struct OpaqueJSString *,struct OpaqueJSValue const * *)" (?popupValue@JSAccessibilityUIElement@WTR@@CAPEBUOpaqueJSValue@@PEBUOpaqueJSContext@@PEAU3@PEAUOpaqueJSString@@PEAPEBU3@@Z)
bin64\TestRunnerInjectedBundle.dll : fatal error LNK1120: 1 unresolved externals
Comment 16 Andres Gonzalez 2019-06-30 14:12:58 PDT
Created attachment 373197 [details]
Patch
Comment 17 Andres Gonzalez 2019-06-30 14:18:05 PDT
(In reply to chris fleizach from comment #15)
> wincairo failure now
> 
>   Creating library lib64\TestRunnerInjectedBundle.lib and object
> lib64\TestRunnerInjectedBundle.exp
> JSAccessibilityUIElement.cpp.obj : error LNK2019: unresolved external symbol
> "public: class JSRetainPtr<struct OpaqueJSString *> __cdecl
> WTR::AccessibilityUIElement::popupValue(void)const "
> (?popupValue@AccessibilityUIElement@WTR@@QEBA?AV?$JSRetainPtr@PEAUOpaqueJSStr
> ing@@@@XZ) referenced in function "private: static struct OpaqueJSValue
> const * __cdecl WTR::JSAccessibilityUIElement::popupValue(struct
> OpaqueJSContext const *,struct OpaqueJSValue *,struct OpaqueJSString
> *,struct OpaqueJSValue const * *)"
> (?popupValue@JSAccessibilityUIElement@WTR@@CAPEBUOpaqueJSValue@@PEBUOpaqueJSC
> ontext@@PEAU3@PEAUOpaqueJSString@@PEAPEBU3@@Z)
> bin64\TestRunnerInjectedBundle.dll : fatal error LNK1120: 1 unresolved
> externals

Fixed now. Perhaps a script to add all the no implemented methods to DRT and WTR for other platforms would be worth the trouble...
Comment 18 WebKit Commit Bot 2019-06-30 18:21:23 PDT
Comment on attachment 373197 [details]
Patch

Clearing flags on attachment: 373197

Committed r246958: <https://trac.webkit.org/changeset/246958>
Comment 19 WebKit Commit Bot 2019-06-30 18:21:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2019-06-30 18:22:21 PDT
<rdar://problem/52433622>
Comment 21 Truitt Savell 2019-07-01 10:24:06 PDT
The changes in https://trac.webkit.org/changeset/246958/webkit

broke inspector/dom/getAccessibilityPropertiesForNode.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom%2FgetAccessibilityPropertiesForNode.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-actual.txt
@@ -296,7 +296,6 @@
     focused: false
     ownedNodeIds.length: 1
     required: false
-    isPopUpButton: true
 
 <select>
     <option>FOO</option>


Is this possibly a rebasline?
Comment 22 Truitt Savell 2019-07-01 14:07:35 PDT
Reverted r246958 for reason:

Broke inspector/dom/getAccessibilityPropertiesForNode.html

Committed r247019: <https://trac.webkit.org/changeset/247019>
Comment 23 Ryan Haddad 2019-07-01 14:45:17 PDT
This passed EWS because the test is marked as a flaky timeout:
LayoutTests/platform/mac/TestExpectations:1063:webkit.org/b/148636 inspector/dom/getAccessibilityPropertiesForNode.html [ Pass Timeout ]

EWS runs tests with the '--skip-failing-tests' argument, which appears to apply to flaky tests. Because of that, this test wasn't run at all pre-commit.
Comment 24 Andres Gonzalez 2019-07-01 19:09:18 PDT
Created attachment 373300 [details]
Patch
Comment 25 Andres Gonzalez 2019-07-01 19:16:07 PDT
inspector/dom/getAccessibilityPropertiesForNode.html caught an issue with my previous patch concerning comboboxes. This latest patch addresses that issue. inspector/dom/getAccessibilityPropertiesForNode.html may still be a flaky test, though.
Comment 26 Andres Gonzalez 2019-07-02 08:49:55 PDT
Created attachment 373325 [details]
Patch
Comment 27 Andres Gonzalez 2019-07-02 08:54:17 PDT
New patch with all changes including fix for inspector/dom/getAccessibilityPropertiesForNode.html.
Comment 28 WebKit Commit Bot 2019-07-02 13:24:30 PDT
Comment on attachment 373325 [details]
Patch

Clearing flags on attachment: 373325

Committed r247071: <https://trac.webkit.org/changeset/247071>
Comment 29 WebKit Commit Bot 2019-07-02 13:24:32 PDT
All reviewed patches have been landed.  Closing bug.