Please see Marco's wonderful blog post about accessible modal dialogs: https://www.marcozehe.de/2015/02/05/advanced-aria-tip-2-accessible-modal-dialogs/ If you follow through all the items there it will be in a great shape.

according to the discussion with Photon onboarding PM/UX, change to P3

Just tried to make a patch based on now ongoing patch of bug 1377289. This is just a WIP. One thing I'm not quite sure is about the behavior of button... Maybe I should not let whitespace and enter trigger button since they're triggered by default and causes some problem like trigger on both keydown and keyup.

Since I've been looking at the a11y for onboarding this week, I have this proposal which essentially completes the a11y work on dialog. @rexboy, hope that's OK? this patch depends on the ones from bug 1377273, bug 1377298 and bug 1377283

Comment on attachment 8893185 [details] [diff] [review] 1377276 proposal v1 Review of attachment 8893185 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I've shown the changes to Verdi and he looks happy as well. Here's the screencast after applied all patches(with my suggest change of bug 1377298) http://recordit.co/hcIizXPjjl Great job Yura! ::: browser/extensions/onboarding/content/onboarding.js @@ +19,5 @@ > const BRAND_SHORT_NAME = Services.strings > .createBundle("chrome://branding/locale/brand.properties") > .GetStringFromName("brandShortName"); > const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count"; > the blank line can be removed @@ +418,5 @@ > + /** > + * A list of all keyboard focusable elements inside the dialog overlay (e.g. > + * buttons, inputs, elements with tabindex). > + */ > + get focusableElms() { can moved to just above the `wrapMoveFocus` method, or integrate in wrapMoveFocus since no one else called this method

Comment on attachment 8893185 [details] [diff] [review] 1377276 proposal v1 Review of attachment 8893185 [details] [diff] [review]: ----------------------------------------------------------------- looks good with one change needed. Thanks for this great patch! ::: browser/extensions/onboarding/content/onboarding.js @@ +661,4 @@ > if (hiddenCheckbox.checked) { > this.hide(); > + } else { > + this.toggleModal(this._overlay.classList.contains("onboarding-opened")); You should move this line right after toggling "onboarding-opened". Otherwise it won't get triggered on closing after checking the hidden checkbox. toggleOverlay() may be better to split up to openOverlay() and closeOverlay() because it starts to mix up on-open and on-close tasks together, just like the hiddenCheckbox case here which is a on-close task.. but we don't necessary to finish that in this bug.

All comments addressed.

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3eaec2fe0d89 add modal dialog semantics and better accessibility for onboarding overlay dialog. r=mossop, gasolin, rexboy

Backed out for for linting failure at onboarding.js:984 and failing browser-chrome's browser_onboarding_accessibility.js: Bug 1377283 https://hg.mozilla.org/integration/mozilla-inbound/rev/abb7505076b5392b0f30ac24df3e9f30898a8741 Bug 1377298 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2eccfad14bc9e298e3d41886159520b0ade6c01 Bug 1377276 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f20d5569dc4f0fb99cc04c854fe11c5e88a51b Bug 1387057 https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa84b4bcd4d7d7626e037dfaf89a617a2b8ba2e Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e6ac32ef78d3c9493a2cfe9313aef9be47b10b77&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel Failure log eslint: https://treeherder.mozilla.org/logviewer.html#?job_id=120897921&repo=mozilla-inbound > TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/extensions/onboarding/content/onboarding.js:984:5 | Unsafe assignment to innerHTML (no-unsanitized/property) Failure log browser-chrome: https://treeherder.mozilla.org/logviewer.html#?job_id=120902393&repo=mozilla-inbound [task 2017-08-04T05:53:12.472863Z] 05:53:12 INFO - TEST-PASS | browser/extensions/onboarding/test/browser/browser_onboarding_accessibility.js | Content should be visible to screen reader - "true" == "true" - [task 2017-08-04T05:53:12.475728Z] 05:53:12 INFO - Buffered messages finished [task 2017-08-04T05:53:12.478768Z] 05:53:12 INFO - TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_accessibility.js | Focus should be on the first tour item - {} == {} - [task 2017-08-04T05:53:12.481486Z] 05:53:12 INFO - Stack trace: [task 2017-08-04T05:53:12.488560Z] 05:53:12 INFO - resource://testing-common/content-task.js line 52 > eval:null:11 [task 2017-08-04T05:53:12.490251Z] 05:53:12 INFO - resource://testing-common/content-task.js:null:53 [task 2017-08-04T05:53:12.493159Z] 05:53:12 INFO - Not taking screenshot here: see the one that was previously logged [task 2017-08-04T05:53:12.496281Z] 05:53:12 INFO - TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_accessibility.js | Overlay button focus state is saved correctly - "undefined" == "true" - [task 2017-08-04T05:53:12.499231Z] 05:53:12 INFO - Stack trace: [task 2017-08-04T05:53:12.504917Z] 05:53:12 INFO - resource://testing-common/content-task.js line 52 > eval:null:13 [task 2017-08-04T05:53:12.508518Z] 05:53:12 INFO - resource://testing-common/content-task.js:null:53 [task 2017-08-04T05:53:12.510420Z] 05:53:12 INFO - Close the dialog and check modal dialog state

The browser tests seem to intermittently cause tab crashes (in consequent tests not the ones that are added). Anecdotally, the culprit seems to be the call to BrowserTestUtils.synthesizeKey. When that part is commented out, the tests work just fine.

(In reply to Yura Zenevich [:yzen] from comment #11) > The browser tests seem to intermittently cause tab crashes (in consequent > tests not the ones that are added). Anecdotally, the culprit seems to be the > call to BrowserTestUtils.synthesizeKey. When that part is commented out, the > tests work just fine. Does this crash on all platforms? What do the stacks look like?

Discussed implications of landing it in bug 1377298, looks like they tests only need to be disabled for debug builds.

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6618d0bc17 add modal dialog semantics and better accessibility for onboarding overlay dialog. r=mossop, gasolin, rexboy

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf94aae2870 for failures in browser_onboarding_accessibility.js like https://treeherder.mozilla.org/logviewer.html#?job_id=123775947&repo=mozilla-inbound

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/401492c55ab4 add modal dialog semantics and better accessibility for onboarding overlay dialog. r=mossop, gasolin, rexboy

Comment on attachment 8893394 [details] [diff] [review] 1377276 proposal v2 This is one of several bugs that make onboarding accessible to keyboard and screen reader users. [Feature/Bug causing the regression]: None [User impact if declined]: Users who use accessibility services or keyboard would not be able to use onboarding. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: * User should be able to dismiss onboarding dialog by pressing ESC key * If onboarding dialog is opened with a keyboard (by focusing and pressing SPACE/ENTER on onboarding overlay button), and then closed, onboarding overlay button should be focused again * If onboarding dialog is opened with a mouse, and then closed, onboarding overlay button should not be focused as it never was. * When onboarding dialog is opened, keyboard focus must always remain in the dialog area. Tabbing inside the dialog should keep the focus on one of its element with wrapping from last to first or first to last when navigating forward or back. [List of other uplifts needed for the feature/fix]: the patch for this feature applied only after bug 1377298 patch. [Is the change risky?]: No [Why is the change risky/not risky?]: Only affects users that use keyboard [String changes made/needed]: None

Comment on attachment 8893394 [details] [diff] [review] 1377276 proposal v2 a11y fix for onboarding. Several big patches here, but we'd like to ensure the photon tours for 56 are accessible.

I can confirm the behavior is as in comment 18. I verified using Fx 57.0a1 (2017-09-01) and Fx 56.0b8, on Windows 10 x64, mac OS X 10.12.6 and Ubuntu 14.04 LTS.

I have verified that this issue is no longer reproducible on Win 10 x64, Win 7 x86, Mac 10.13, & Ubuntu 16.04 x32 with Firefox 58.