Bug 126685

Summary: AX: Merge layout test from Mac and GTK checking accessibility roles
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, ap, buildbot, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, rniwa, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 126689    
Bug Blocks:    
Attachments:
Description Flags
Patch proposal
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch proposal none

Description Mario Sanchez Prada 2014-01-09 02:21:31 PST
As it was discussed in bug 125525 (see https://bugs.webkit.org/show_bug.cgi?id=125525#c5), it looks like a good idea to merge platform/mac/accessibility/role-subrole-roledescription.html and platform/gtk/accessibility/roles-exposed.html into a single test that handles the platform-specific differences gracefully, instead of having two -almost- duplicated HTML files for the same purpose.
Comment 1 Radar WebKit Bug Importer 2014-01-09 02:21:43 PST
<rdar://problem/15780752>
Comment 2 Mario Sanchez Prada 2014-01-09 03:53:05 PST
Created attachment 220711 [details]
Patch proposal

This patch merges the two tests in one, following the idea (or my interpretation of it) suggested by Chris in bug 125525 (see https://bugs.webkit.org/show_bug.cgi?id=125525#c4).

Compared to the original test from the Mac, the differences now are:

* A new data attribute is added to specify which platforms should care about a given element, or ignore it ('data-platform'), specifying a comma-separated list of platforms as returned by accessibilityController.platformName ("atk", "mac" or "win". See bug 126689). If the current "accessibility platform" is not listed for an element, it will be ignored and nothing will be checked about it. See some examples:

<a data-platform="atk,mac" data-role="AXLink" data-subrole="" data-roledescription="link" href="#" data-note="[href]" class="ex">X</a>
<input data-platform="mac" type="color" value="X" data-role="AXColorWell" data-subrole="" data-roledescription="color well" class="ex" data-note="[type='color']">


* To handle the case where the expected role would NOT be the same for all the platforms, a new 'data-role-<platform>' attribute can be added to an element. See some examples:

<dl data-platform="atk,mac" data-role-atk="AXDescriptionList" data-role-mac="AXList" data-subrole="AXDescriptionList" data-alternatesubrole="AXDefinitionList" data-roledescription="description list" class="ex">
    <dt data-platform="atk,mac" data-role-atk="AXDescriptionTerm" data-role-mac="AXGroup" data-subrole="AXTerm" data-roledescription="term" class="ex">X</dt>
    <dd data-platform="atk,mac" data-role-atk="AXDescriptionValue" data-role-mac="AXGroup" data-subrole="AXDescription" data-roledescription="description" class="ex">X</dd>
</dl>


* To handle the case where the expected role would be the same for more than one platform, the 'data-role' attribute can still be used (see the first examples above), as the role checking code will fallback to it in case it could not find a 'data-role-<platform>'one:

        expectedRole = "";
        if (el.hasAttribute('data-role-' + currentPlatform)) {
            expectedRole = el.getAttribute('data-role-' + currentPlatform);
        } else if (el.hasAttribute('data-role')) {
            expectedRole = el.getAttribute('data-role');
        }


* This test depends on bug 126689 because it expects accessibilityController.platformName instead of testRunner.platformName, and also because it expects "atk" for both the GTK and EFL ports, so it can compare it against the values included in the new 'data-platform' attribute:

    var currentPlatform = accessibilityController.platformName;
    [...]
    for (var i = 0, c = examples.length; i < c; i++) {
        el = examples[i];

        supportedPlatforms = el.getAttribute('data-platform');
        if (!supportedPlatforms || supportedPlatforms.indexOf(currentPlatform) == -1)
            continue;

        [...]
    }


I have carefully changed the code using the Mac test as a base and only changing bits that should not have altered the logic (and the output) in the mac port, but I can't be 100% sure that I did not break anything there, so I'd rather set the r? flag once bug 126689 has been fixed and the EWS for this test have been run. It "should work fine", though :)

In the meanwhile, any feedback is welcome of course!
Comment 3 chris fleizach 2014-01-09 08:58:30 PST
Comment on attachment 220711 [details]
Patch proposal

i think this plan looks good
Comment 4 Build Bot 2014-01-13 04:05:27 PST
Comment on attachment 220711 [details]
Patch proposal

Attachment 220711 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5125621755674624

New failing tests:
accessibility/roles-exposed.html
Comment 5 Build Bot 2014-01-13 04:05:31 PST
Created attachment 221025 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-01-13 04:25:22 PST
Comment on attachment 220711 [details]
Patch proposal

Attachment 220711 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5069476198350848

New failing tests:
accessibility/roles-exposed.html
Comment 7 Build Bot 2014-01-13 04:25:25 PST
Created attachment 221027 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-01-13 04:56:17 PST
Comment on attachment 220711 [details]
Patch proposal

Attachment 220711 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6013784887918592

New failing tests:
accessibility/roles-exposed.html
Comment 9 Build Bot 2014-01-13 04:56:20 PST
Created attachment 221029 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Mario Sanchez Prada 2014-01-13 05:23:17 PST
So glad to see that the only text diff in the mac seems to be this one:

-This tests that native elements and ARIA overrides result in the expected role, subrole, and role description.
+This tests that native elements and ARIA overrides result in the expected role, subrole and role description.

I'll update the mac specific expectations fixing that typo now and upload a new patch
Comment 11 Mario Sanchez Prada 2014-01-13 05:25:52 PST
Created attachment 221030 [details]
Patch proposal
Comment 12 WebKit Commit Bot 2014-01-15 05:11:39 PST
Comment on attachment 221030 [details]
Patch proposal

Clearing flags on attachment: 221030

Committed r162067: <http://trac.webkit.org/changeset/162067>
Comment 13 WebKit Commit Bot 2014-01-15 05:11:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Alexey Proskuryakov 2014-01-15 10:19:05 PST
This patch broke accessibility/roles-exposed.html on Mac Mavericks:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Froles-exposed.html
Comment 15 Alexey Proskuryakov 2014-01-15 10:56:58 PST
Corrected Mac test results in <http://trac.webkit.org/r162082>.
Comment 16 Mario Sanchez Prada 2014-03-24 03:29:05 PDT
*** Bug 125585 has been marked as a duplicate of this bug. ***