Bug 88911 – Add test cases for `header`/`footer` AXRoleDescription

• Component: Accessibility
• Status: RESOLVED FIXED

[Mike West]

There's a bit of fallback logic regarding `header` and `footer` elements that changes their meaning depending on the context in which they're found. So far as I can tell, we're not testing this behavior yet.

[Mike West]

Created attachment 147156 [details]
Patch

[Mike West]

CCing Dominic: this is the first a11y test I've written. Would you mind poking at it a bit to see if I've done things correctly?

[WebKit Review Bot]

Comment on attachment 147156 [details]
Patch

Attachment 147156 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12942742

New failing tests:
accessibility/footer.html
accessibility/header.html

[WebKit Review Bot]

Created attachment 147210 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dominic Mazzoni]

Comment on attachment 147156 [details]
Patch

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

> LayoutTests/accessibility/footer-expected.txt:2
> +PASS accessibilityController.focusedElement.roleDescription is "AXRoleDescription: footer"

This will only pass on Mac, you should either:

1. Move the expectations to LayoutTests/platform/mac/accessibility/footer-expected.txt and rewrite it so that the role names like AXGroup, etc. are only in the expectations and not in the test file
2. Or, just move the whole test to LayoutTests/platform/mac/accessibility/footer.html

BTW, I would really like to see a way to write "cross-platform" accessibility tests that test for the WebCore role rather than the platform-specific role, I just haven't had a chance to think about how to do that refactoring yet.

> LayoutTests/accessibility/footer.html:16
> +      function log(str) {

Use consistent indentation - preferably 4 spaces throughout.

[Mike West]

Created attachment 147585 [details]
Patch

[Mike West]

Created attachment 147586 [details]
Four spaces.

[Mike West]

(In reply to comment #5)
> (From update of attachment 147156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147156&action=review
> 
> > LayoutTests/accessibility/footer-expected.txt:2
> > +PASS accessibilityController.focusedElement.roleDescription is "AXRoleDescription: footer"
> 
> This will only pass on Mac, you should either:
> 
> 1. Move the expectations to LayoutTests/platform/mac/accessibility/footer-expected.txt and rewrite it so that the role names like AXGroup, etc. are only in the expectations and not in the test file
> 2. Or, just move the whole test to LayoutTests/platform/mac/accessibility/footer.html

Done (#2).

> BTW, I would really like to see a way to write "cross-platform" accessibility tests that test for the WebCore role rather than the platform-specific role, I just haven't had a chance to think about how to do that refactoring yet.

I've only taken a very quick look at the code, but it seems like it should be possible to set up a mechanism to test the "real" role in DumpRenderTree by adding some methods to AccessibilityUIElement. Maybe with an enum hanging around that's mapped onto ARIARoleMap?

Seomthing like AccessibilityUIElement::isRole([enum]) and AccessibilityUIElement::isRoleDescription([enum])?

Is there a bug for this sort of refactoring? It sounds like it'd be useful...

> > LayoutTests/accessibility/footer.html:16
> > +      function log(str) {
> 
> Use consistent indentation - preferably 4 spaces throughout.

Ugh. Thanks. I was even extra careful this time. :)

[Dominic Mazzoni]

Looks good now. Chris Fleizach should be able to give you the official review.

(In reply to comment #8)
> > BTW, I would really like to see a way to write "cross-platform" accessibility tests that test for the WebCore role rather than the platform-specific role, I just haven't had a chance to think about how to do that refactoring yet.
> 
> I've only taken a very quick look at the code, but it seems like it should be possible to set up a mechanism to test the "real" role in DumpRenderTree by adding some methods to AccessibilityUIElement. Maybe with an enum hanging around that's mapped onto ARIARoleMap?
> 
> Seomthing like AccessibilityUIElement::isRole([enum]) and AccessibilityUIElement::isRoleDescription([enum])?
> 
> Is there a bug for this sort of refactoring? It sounds like it'd be useful...

Added a new bug:
https://bugs.webkit.org/show_bug.cgi?id=89103

isRole wouldn't work very well, we'd have to expose the enum to JavaScript. Instead we should just expose the WebCore role name as a string in JavaScript, and make it a compile error to add a new role without adding the role name.

I don't think we need to have role descriptions in cross-platform tests, that's a Mac-only concept. The WebCore role enum should encompass all distinct roles, and each platform should map a role to whatever makes sense on that platform - on Mac one role might map to a {role, roleDescription} tuple.

[Mike West]

(In reply to comment #9)
> Looks good now. Chris Fleizach should be able to give you the official review.

Hi Chris! Adding you to the CC list... Low priority, but nice to land at some point.

Moving the rest of the conversation to the other bug. Thanks for creating it, Dominic!

[chris fleizach]

Comment on attachment 147586 [details]
Four spaces.

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

looks good otherwise from change log nit. ready to cq+ once that's fixed

> LayoutTests/ChangeLog:8
> +        as "banner" or "footer" in the general case, and  fall back to

extra space after "and  fall"

[Mike West]

Created attachment 147641 [details]
Patch

[Mike West]

Comment on attachment 147641 [details]
Patch

Patch upload, now with 100% fewer whitespace typos! Probably! :)  CQ?

[Build Bot]

Comment on attachment 147641 [details]
Patch

Attachment 147641 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12953670

[Mike West]

(In reply to comment #14)
> (From update of attachment 147641 [details])
> Attachment 147641 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12953670

Since the patch doesn't touch any WebCore code, I doubt this is related.

[chris fleizach]

Comment on attachment 147641 [details]
Patch

looks good

[WebKit Review Bot]

Comment on attachment 147641 [details]
Patch

Clearing flags on attachment: 147641

Committed r120383: <http://trac.webkit.org/changeset/120383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

The newly added tests are failing on Lion.

http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r120384%20(131)/results.html

I'm reverting.

[WebKit Review Bot]

Re-opened since this is blocked by 89229

[Mike West]

(In reply to comment #19)
> The newly added tests are failing on Lion.

Ugh. I moved the tests to `platform/mac/accessibility` but didn't update the relative path to the JavaScript files. Apologies.

I'll throw a patch up to fix the issue in a moment.

[Mike West]

Created attachment 147972 [details]
Patch h that fixes directories.

[Mike West]

Comment on attachment 147972 [details]
Patch h that fixes directories.

This works correctly on my Lion machine locally. I actually tested it this time. :)

CQ?

[WebKit Review Bot]

Comment on attachment 147972 [details]
Patch h that fixes directories.

Clearing flags on attachment: 147972

Committed r120538: <http://trac.webkit.org/changeset/120538>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.