Closed Bug 1377283 Opened 7 years ago Closed 7 years ago

[a11y] "Getting started with Nightly" dialog's close button must be accessible.

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

(1 file, 1 obsolete file)

Right now it's just a span.

It must do the following:

* be implemented as BUTTON tag
* be present in the tab order (Fred and I talked about it being the last one within the dialog)
* have a clear focused state (when it has a keyboard focus)
* have an internationalized label (e.g. alt text for the image)
according to the discussion with Photon onboarding PM/UX, change to P3
Priority: -- → P3
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Assignee: nobody → rexboy
(In reply to Yura Zenevich [:yzen] from comment #0)
> Right now it's just a span.
> 
> It must do the following:
> 
> * be implemented as BUTTON tag
This has been done in the bug 1377439.
> * be present in the tab order (Fred and I talked about it being the last one
> within the dialog)
> * have a clear focused state (when it has a keyboard focus)
> * have an internationalized label (e.g. alt text for the image)
This could be done with the Bug 1381366, sharing the same string with the tooltip
(In reply to Fischer [:Fischer] from comment #2)

> This could be done with the Bug 1381366, sharing the same string with the
> tooltip

For this button the tooltip should be "Close"
(In reply to Verdi [:verdi] from comment #3)
> (In reply to Fischer [:Fischer] from comment #2)
> 
> > This could be done with the Bug 1381366, sharing the same string with the
> > tooltip
> 
> For this button the tooltip should be "Close"
OK, I will update the bug 1381366 title to describe "Update the tooltips and arial-label for the notification close button and the overlay close button"

So for this bug, the remaining jobs are: 
* be present in the tab order (Fred and I talked about it being the last one within the dialog)
* have a clear focused state (when it has a keyboard focus)
(In reply to Fischer [:Fischer] from comment #4)
> OK, I will update the bug 1381366 title to describe "Update the tooltips and
> arial-label for the notification close button and the overlay close button"
> 
> So for this bug, the remaining jobs are: 
> * be present in the tab order (Fred and I talked about it being the last one
> within the dialog)
> * have a clear focused state (when it has a keyboard focus)
Because of the bug 1384822, I will leave the arial-label job here.
So for this bug, the remaining jobs are: 
* be present in the tab order (Fred and I talked about it being the last one
within the dialog)
* have a clear focused state (when it has a keyboard focus)
* have an internationalized label (e.g. alt text for the image)
Target Milestone: --- → Firefox 57
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
Attached patch 1377283 close button a11y (obsolete) — Splinter Review
@rexboy, hope it's ok I made a patch for this (going through the a11y this week).
Attachment #8892510 - Flags: review?(gasolin)
Thanks for the help. I did some study for accessibility but haven't start making the patch yet, so just feel free to do it.
Setting assignee to Yura.
Assignee: rexboy → yzenevich
Comment on attachment 8892510 [details] [diff] [review]
1377283 close button a11y

Review of attachment 8892510 [details] [diff] [review]:
-----------------------------------------------------------------

since we want the close button to be the last element to be selected, we can move onboarding-overlay-close-btn down to the last element

::: browser/extensions/onboarding/content/onboarding.js
@@ +864,5 @@
>        </div>
>      `;
>  
> +    div.querySelector("#onboarding-overlay-close-btn").title =
> +      this._bundle.GetStringFromName("onboarding.close");

we've added the close button text in bug 1381366, please rebase the patch

::: browser/extensions/onboarding/locales/en-US/onboarding.properties
@@ +16,5 @@
>  
>  # LOCALIZATION NOTE(onboarding.complete): This string is used to describe an
>  # onboarding tour item that is complete.
>  onboarding.complete=Complete
> +# LOCALIZATION NOTE(onboarding.close): Onboarding dialog and notification close button label.

no need since the string is added
Attachment #8892510 - Flags: review?(gasolin)
This should take care of all the comments.
Attachment #8892510 - Attachment is obsolete: true
Attachment #8892971 - Flags: review?(gasolin)
Comment on attachment 8892971 [details] [diff] [review]
1377283 close button a11y v2

Review of attachment 8892971 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks!
Attachment #8892971 - Flags: review?(gasolin) → review+
Attachment #8892971 - Flags: review?(dtownsend)
Attachment #8892971 - Flags: review?(dtownsend) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e1205b2d4b
making Close onboarding dialog button accessible. r=gasolin, mossop
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)
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c54f94dd60d
making Close onboarding dialog button accessible. r=gasolin, mossop
This one dis not cause eslint failure so landing separately.
Flags: needinfo?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/2c54f94dd60d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi Yura Zenevich,
Could you please provide the steps to verify this fix? Thanks
Flags: needinfo?(yzenevich)
it will be easier to check after bug 1377276, bug 1377298 are landed

* use tab key to navigate and make sure the close button is selected as the last item
* when the close button is selected by key, it should have same Visual as mouse hover
* when selected the close button, can use space key to close the overlay
Flags: needinfo?(yzenevich)
Comment on attachment 8892971 [details] [diff] [review]
1377283 close button a11y 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?]: No, markup/css changes only
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: comment 17
[List of other uplifts needed for the feature/fix]: not for this bug, but all onboarding accessibility bugs are listed in bug 1377300
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects users that use keyboard
[String changes made/needed]: None
Attachment #8892971 - Flags: approval-mozilla-beta?
Comment on attachment 8892971 [details] [diff] [review]
1377283 close button a11y v2

CSS only fix, let's uplift this for onboarding a11y
Attachment #8892971 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta due to conflicts from bug 1385702.
Flags: needinfo?(yzenevich)
posted try with rebased commits in bug 1377300
Flags: needinfo?(yzenevich)
I can confirm this issue is fixed, I reproduced it with Fx 56.0a1 (build ID: 20170629030206) on Windows 10 x64.
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.

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: qe-verify+
I have verified that this issue works as expected 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.