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

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

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)

Comment 1

[Francis Lee [:frlee]]

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

Comment 2

[Fischer [:Fischer]]

(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

Comment 3

[Verdi [:verdi] Best to slack me]

(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"

Comment 4

[Fischer [:Fischer]]

(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)

Comment 5

[Fischer [:Fischer]]

(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)

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1377283 close button a11y

@rexboy, hope it's ok I made a patch for this (going through the a11y this week).

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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.

Comment 8

[[e-mail]]

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

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1377283 close button a11y v2

This should take care of all the comments.

Comment 10

[[e-mail]]

Comment on attachment 8892971 [details] [diff] [review]
1377283 close button a11y v2

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

looks good, thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e1205b2d4b
making Close onboarding dialog button accessible. r=gasolin, mossop

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c54f94dd60d
making Close onboarding dialog button accessible. r=gasolin, mossop

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

This one dis not cause eslint failure so landing separately.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2c54f94dd60d

Comment 16

[Abe - QA (:Abe_LV)]

Hi Yura Zenevich,
Could you please provide the steps to verify this fix? Thanks

Comment 17

[[e-mail]]

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

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8892971 [details] [diff] [review]
1377283 close button a11y v2

CSS only fix, let's uplift this for onboarding a11y

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta due to conflicts from bug 1385702.

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

posted try with rebased commits in bug 1377300

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/10a3f2c7c48c

Comment 23

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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!

Comment 24

[Justin [:JW_SoftvisionQA]]

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.