Closed Bug 1377276 Opened 2 years ago Closed 2 years ago

[a11y] The "Getting started with Nightly" popup dialog must be accessible and employ the necessary semantics.

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox56 --- verified
firefox57 --- verified
firefox58 --- verified

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access, Whiteboard: [photon-onboarding])

Attachments

(2 files, 1 obsolete file)

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
Priority: -- → P3
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Assignee: nobody → rexboy
Target Milestone: --- → Firefox 57
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
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.
Attached patch 1377276 proposal v1 (obsolete) — Splinter Review
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
Attachment #8893185 - Flags: review?(rexboy)
Attachment #8893185 - Flags: review?(gasolin)
Attachment #8893185 - Flags: review?(dtownsend)
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
Attachment #8893185 - Flags: review?(gasolin) → review+
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.
Attachment #8893185 - Flags: review?(rexboy) → review+
All comments addressed.
Attachment #8893185 - Attachment is obsolete: true
Attachment #8893185 - Flags: review?(dtownsend)
Attachment #8893394 - Flags: review?(dtownsend)
Attachment #8893394 - Flags: review?(dtownsend) → review+
Assignee: rexboy → yzenevich
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
Flags: needinfo?(yzenevich)
Duplicate of this bug: 1387463
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.
Flags: needinfo?(yzenevich) → needinfo?(dtownsend)
(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?
Flags: needinfo?(dtownsend) → needinfo?(yzenevich)
Discussed implications of landing it in bug 1377298, looks like they tests only need to be disabled for debug builds.
Flags: needinfo?(yzenevich)
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
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
https://hg.mozilla.org/mozilla-central/rev/401492c55ab4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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
Attachment #8893394 - Flags: approval-mozilla-beta?
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.
Attachment #8893394 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
You need to log in before you can comment on or make changes to this bug.