Closed Bug 1377298 Opened 7 years ago Closed 7 years ago

[a11y] onboarding-tour-list widget (e.g. tabs) should be accessible and implemented semantically as tabs.

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

(3 files, 4 obsolete files)

Marco's article is great:

https://www.marcozehe.de/2013/02/02/advanced-aria-tip-1-tabs-in-web-apps/

Also a good example of a very nice implementation within firefox is about:debugging
according to the discussion with Photon onboarding PM/UX, change to P3
Priority: -- → P3
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
* Once focus is on dialog you can do the following (keyboard-wise):
** tab to/between tabs in the tablist on the left
** press space/enter to activate a currently focused tab
** arrow up/down to move between tabs

* The widget now also has proper tablist/tab/tabpanel semantics.
* Completed styling now has proper textual alternative description.
Attachment #8890047 - Flags: review?(dtownsend)
Note: this bug depends on the patch from bug 1377285 to properly apply for CSS changes.
Depends on: 1377285
Attachment #8890047 - Flags: ui-review?(mverdi)
(In reply to Yura Zenevich [:yzen] from comment #2)
> Created attachment 8890047 [details] [diff] [review]
> 1377298 tab semantics and keyboard navigation
> 
> * Once focus is on dialog you can do the following (keyboard-wise):
> ** tab to/between tabs in the tablist on the left
> ** press space/enter to activate a currently focused tab
> ** arrow up/down to move between tabs
> 
> * The widget now also has proper tablist/tab/tabpanel semantics.
> * Completed styling now has proper textual alternative description.

Also keyboard focus styling is now consistent with the action buttons for each tab (e.g. bug 1377285)
Comment on attachment 8890047 [details] [diff] [review]
1377298 tab semantics and keyboard navigation

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

Awesome work, thank you for this, in particular for adding the tests so we don't regress this. r+ assuming verdi is ok with the UI bits.
Attachment #8890047 - Flags: review?(dtownsend) → review+
Target Milestone: --- → Firefox 57
Comment on attachment 8890047 [details] [diff] [review]
1377298 tab semantics and keyboard navigation

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

The patch works well, but it need rebase before land.

I'll build a version and help Verdi for ui-review

::: browser/extensions/onboarding/content/onboarding.js
@@ +491,5 @@
> +        // activation if there is a click handler for the target.
> +        target.click();
> +        break;
> +      case "ArrowUp":
> +        // Go to and focus on the previous tab if it's available.

need do event.stopPropagation for up and down key navigation, or the page below overlay will be scrolled as well (check the about:newtab)
Fred just showed me what this looks like and I think there are a few issues to fix. Mostly it seems the problem is that this is visible to mouse users. The buttons should not be outlined when you click on them. It also seems that as a side effect, the CTAs on each panel also have this outline on hover.
Updated the patch to deal with separate focus styling for mouse and keyboard.
Attachment #8890047 - Attachment is obsolete: true
Attachment #8890047 - Flags: ui-review?(mverdi)
Attachment #8892435 - Flags: ui-review?(mverdi)
Attachment #8892435 - Flags: review?(gasolin)
Attachment #8892435 - Flags: review?(dtownsend)
Comment on attachment 8892435 [details] [diff] [review]
1377298 tab semantics and keyboard navigation v2

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

as previous comment, please rebase to latest branch and add the evt.stopPropagation for space/enter/up/down key navigation
Attachment #8892435 - Flags: review?(gasolin)
Hopefully all comments addressed.
Attachment #8892435 - Attachment is obsolete: true
Attachment #8892435 - Flags: ui-review?(mverdi)
Attachment #8892435 - Flags: review?(dtownsend)
Attachment #8893045 - Flags: review?(gasolin)
Attachment #8893045 - Flags: review?(dtownsend)
Attachment #8893045 - Flags: a11y-review?(mverdi)
Uploaded old file :/
Attachment #8893045 - Attachment is obsolete: true
Attachment #8893045 - Flags: review?(gasolin)
Attachment #8893045 - Flags: review?(dtownsend)
Attachment #8893045 - Flags: a11y-review?(mverdi)
Attachment #8893048 - Flags: ui-review?(mverdi)
Attachment #8893048 - Flags: review?(gasolin)
Attachment #8893048 - Flags: review?(dtownsend)
Comment on attachment 8893048 [details] [diff] [review]
1377298 tab semantics and keyboard navigation v3

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

It works well now! though for key navigation I suggest calling `this.handleClick(target);` instead of calling this.gotoPage directly, since some tour item has special treatment besides this.gotoPage, 
For example, click on sync and default browser tour should set tour as complete.
calling this.gotoPage directly will lose these actions.

::: browser/extensions/onboarding/content/onboarding.js
@@ +524,5 @@
> +      case "Enter":
> +        // Assume that the handle function should be identical for keyboard
> +        // activation if there is a click handler for the target.
> +        if (target.classList.contains("onboarding-tour-item")) {
> +          this.gotoPage(target.id);

this.handleClick(target);

@@ +533,5 @@
> +        // Go to and focus on the previous tab if it's available.
> +        targetIndex = this._tourItems.indexOf(target);
> +        if (targetIndex > 0) {
> +          let previous = this._tourItems[targetIndex - 1];
> +          this.gotoPage(previous.id);

this.handleClick(previous);

@@ +543,5 @@
> +        // Go to and focus on the next tab if it's available.
> +        targetIndex = this._tourItems.indexOf(target);
> +        if (targetIndex > -1 && targetIndex < this._tourItems.length - 1) {
> +          let next = this._tourItems[targetIndex + 1];
> +          this.gotoPage(next.id);

this.handleClick(next);
Attachment #8893048 - Flags: review?(gasolin) → feedback+
Comment on attachment 8893048 [details] [diff] [review]
1377298 tab semantics and keyboard navigation v3

Thanks Yura - looks good.
Attachment #8893048 - Flags: ui-review?(mverdi) → ui-review+
All comments addressed.
Attachment #8893048 - Attachment is obsolete: true
Attachment #8893048 - Flags: review?(dtownsend)
Attachment #8893385 - Flags: review?(dtownsend)
Attachment #8893385 - Flags: review?(dtownsend) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c848b96019
improve semantics and keyboard accessibility of tour tabs UI in onboarding overlay. r=mossop, gasolin
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/952b32c08bc7
improve semantics and keyboard accessibility of tour tabs UI in onboarding overlay. r=mossop, gasolin
This one dis not cause eslint failure so landing separately.
Flags: needinfo?(yzenevich)
Blocks: 1387463
Backed out for failing browser_onboarding_notification_2.js on Windows 8 debug after asserting:

https://hg.mozilla.org/integration/mozilla-inbound/rev/80ad277485d24e3fb2a4040bde3b1676ee381ea4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=952b32c08bc7373bac4298865b9ff1f86604714d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120979702&repo=mozilla-inbound

07:33:27     INFO - TEST-START | browser/extensions/onboarding/test/browser/browser_onboarding_notification_2.js
07:33:27     INFO - GECKO(4328) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x140084,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
07:33:27     INFO - GECKO(4328) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x140078,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
07:33:28     INFO - GECKO(4328) | --DOMWINDOW == 2 (00000021F8618000) [pid = 1660] [serial = 2] [outer = 0000000000000000] [url = about:blank]
07:33:28     INFO - GECKO(4328) | ++DOCSHELL 000000DDA9D43000 == 5 [pid = 4328] [id = {6c02660f-6274-4b04-90e1-c6c69ddf8dab}]
07:33:28     INFO - GECKO(4328) | ++DOMWINDOW == 11 (000000DDAA205800) [pid = 4328] [serial = 14] [outer = 0000000000000000]
07:33:28     INFO - GECKO(4328) | ++DOMWINDOW == 12 (000000DDAA21A000) [pid = 4328] [serial = 15] [outer = 000000DDAA205800]
07:33:28     INFO - GECKO(4328) | --DOCSHELL 0000006F9417F000 == 1 [pid = 4056] [id = {5187cbcb-5f70-4403-9a3e-b8db060b446b}]
07:33:28     INFO - GECKO(4328) | --DOMWINDOW == 9 (0000006F97A31800) [pid = 4056] [serial = 3] [outer = 0000000000000000] [url = about:blank]
07:33:28     INFO - GECKO(4328) | --DOMWINDOW == 8 (0000006F9A051800) [pid = 4056] [serial = 4] [outer = 0000000000000000] [url = about:blank]
07:33:28     INFO - GECKO(4328) | ++DOMWINDOW == 13 (000000DDAAB39000) [pid = 4328] [serial = 16] [outer = 000000DDAA205800]
07:33:30     INFO - GECKO(4328) | --DOMWINDOW == 12 (000000DDAF25E000) [pid = 4328] [serial = 10] [outer = 0000000000000000] [url = about:blank]
07:33:30     INFO - GECKO(4328) | --DOMWINDOW == 11 (000000DDA59D9800) [pid = 4328] [serial = 2] [outer = 0000000000000000] [url = about:blank]
07:33:31     INFO - GECKO(4328) | --DOMWINDOW == 7 (0000006F97DC4800) [pid = 4056] [serial = 5] [outer = 0000000000000000] [url = about:blank]
07:33:34     INFO - GECKO(4328) | --DOMWINDOW == 10 (000000DDAA21A000) [pid = 4328] [serial = 15] [outer = 0000000000000000] [url = about:blank]
07:33:35     INFO - GECKO(4328) | --DOCSHELL 0000004725B51800 == 0 [pid = 2496] [id = {7ef6be16-0fa6-4311-8e8f-d1c627d1a357}]
07:33:35     INFO - GECKO(4328) | --DOMWINDOW == 6 (0000006F97A30000) [pid = 4056] [serial = 9] [outer = 0000000000000000] [url = about:blank]
07:33:35     INFO - GECKO(4328) | --DOMWINDOW == 5 (0000006F97535800) [pid = 4056] [serial = 8] [outer = 0000000000000000] [url = about:blank]
07:33:35     INFO - GECKO(4328) | --DOMWINDOW == 4 (0000006F9A067800) [pid = 4056] [serial = 6] [outer = 0000000000000000] [url = about:blank]
07:33:35     INFO - GECKO(4328) | --DOMWINDOW == 3 (000000472480B000) [pid = 2496] [serial = 1] [outer = 0000000000000000] [url = about:blank]
07:33:35     INFO - GECKO(4328) | --DOMWINDOW == 2 (0000004725B52000) [pid = 2496] [serial = 3] [outer = 0000000000000000] [url = about:blank]
07:33:38     INFO - GECKO(4328) | --DOMWINDOW == 3 (0000006F94187000) [pid = 4056] [serial = 7] [outer = 0000000000000000] [url = about:newtab]
07:33:39     INFO - GECKO(4328) | --DOMWINDOW == 1 (0000004725B58000) [pid = 2496] [serial = 4] [outer = 0000000000000000] [url = about:blank]
07:33:39     INFO - GECKO(4328) | --DOMWINDOW == 0 (0000004724818000) [pid = 2496] [serial = 2] [outer = 0000000000000000] [url = about:blank]
07:33:41     INFO - GECKO(4328) | --DOMWINDOW == 2 (0000006F99733000) [pid = 4056] [serial = 10] [outer = 0000000000000000] [url = about:newtab]
07:37:57     INFO - TEST-INFO | started process screenshot
07:37:57     INFO - TEST-INFO | screenshot: exit 0
07:37:57     INFO - Buffered messages logged at 07:33:27
07:37:57     INFO - Entering test bound test_not_show_notification_for_completed_tour
07:37:57     INFO - Buffered messages logged at 07:33:28
07:37:57     INFO - Console message: [JavaScript Error: "remote browser crashed while on about:blank
07:37:57     INFO - " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
07:37:57     INFO - e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
07:37:57     INFO - EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:668:5
07:37:57     INFO - interposeProperty/desc.value@jar:file:///C:/slave/test/build/application/firefox/omni.ja!/components/multiprocessShims.js:157:52
07:37:57     INFO - e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
07:37:57     INFO - testInit@chrome://mochikit/content/browser-test.js:101:5
07:37:57     INFO - setTimeout handler*@chrome://mochikit/content/browser-test.js:25:3
07:37:57     INFO - 
07:37:57     INFO - Buffered messages logged at 07:34:07
07:37:57     INFO - Console message: 1501857247552	Toolkit.Telemetry	WARN	TelemetryStorage::_scanArchive - have seen this id before: 4f18adce-da05-4c88-9982-939fef82d6ae, overwrite: false
07:37:57     INFO - Buffered messages logged at 07:34:57
07:37:57     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 2
07:37:57     INFO - Buffered messages logged at 07:36:27
07:37:57     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
07:37:57     INFO - Buffered messages finished
07:37:57     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_notification_2.js | Test timed out -
Flags: needinfo?(yzenevich)
Hi Yura, please take a look on bug 1390042 while rebasing, it will conflict with this patch
just ping me if you need a favor to land this bug
Depends on: 1390042
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 #23)
> 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)
(In reply to Dave Townsend [:mossop] from comment #24)
> (In reply to Yura Zenevich [:yzen] from comment #23)
> > 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?

The treeherder job with failure in comment 21 has a failure in Win8. I can intermittently reproduce on mac. The stack does not say much https://treeherder.mozilla.org/logviewer.html#?job_id=120979702&repo=mozilla-inbound&lineNumber=12440 . There's an assertion that happens some time before the tab crashes so perhaps this issue is specific to debug builds only.. The keyboard tests on osx should be skipped anyways as full keyboard a11y is something that is a system wide pref that we can't set as part of the try.
Flags: needinfo?(yzenevich)
Let's go ahead and land this and the other bug with the tests disabled and file a bug to track getting them working. needinfo me on the new bug you create and I will find someone to help track it down.
(In reply to Dave Townsend [:mossop] from comment #26)
> Let's go ahead and land this and the other bug with the tests disabled and
> file a bug to track getting them working. needinfo me on the new bug you
> create and I will find someone to help track it down.

Sounds good, I will run 1 more experiment with disabling only in debug to see if it can at least run in regular builds.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec85030ef283
improve semantics and keyboard accessibility of tour tabs UI in onboarding overlay. r=mossop, gasolin
had to backthis out from m-i since this conflicts when merging to m-c like:

warning: conflicts while merging browser/extensions/onboarding/locales/en-US/onboarding.properties! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/onboarding/test/browser/browser.ini! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/onboarding/test/browser/browser_onboarding_tours.js! (edit, then use 'hg resolve --mark')
Flags: needinfo?(yzenevich)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62c399b27d0a
Backed out changeset ec85030ef283 for conflict with m-i to m-c merge
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7476ca517e2b
improve semantics and keyboard accessibility of tour tabs UI in onboarding overlay. r=mossop, gasolin
(In reply to Carsten Book [:Tomcat] from comment #30)
> had to backthis out from m-i since this conflicts when merging to m-c like:
> 
> warning: conflicts while merging
> browser/extensions/onboarding/locales/en-US/onboarding.properties! (edit,
> then use 'hg resolve --mark')
> warning: conflicts while merging
> browser/extensions/onboarding/test/browser/browser.ini! (edit, then use 'hg
> resolve --mark')
> warning: conflicts while merging
> browser/extensions/onboarding/test/browser/browser_onboarding_tours.js!
> (edit, then use 'hg resolve --mark')

Looks like bug 1366056 patch was backed out before this stuck, re-landing.
Flags: needinfo?(yzenevich)
Backed out for failing browser-chrome's browser/extensions/onboarding/test/browser/browser_onboarding_keyboard.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8aded3ffc82422e078e1149f121d6988855089

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7476ca517e2be029f24bda7a42299ab6816feb0f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123877917&repo=mozilla-inbound

[task 2017-08-17T14:35:21.287296Z] 14:35:21     INFO - TEST-PASS | browser/extensions/onboarding/test/browser/browser_onboarding_keyboard.js | Active item should have aria-selected set to true and inactive to false - "false" == "false" - 
[task 2017-08-17T14:35:21.291024Z] 14:35:21     INFO - TEST-PASS | browser/extensions/onboarding/test/browser/browser_onboarding_keyboard.js | Active item should have aria-selected set to true and inactive to false - "false" == "false" - 
[task 2017-08-17T14:35:21.293268Z] 14:35:21     INFO - Buffered messages finished
[task 2017-08-17T14:35:21.301536Z] 14:35:21     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_keyboard.js | Active item should have aria-selected set to true and inactive to false - "true" == "false" - 
[task 2017-08-17T14:35:21.304506Z] 14:35:21     INFO - Stack trace:
[task 2017-08-17T14:35:21.306963Z] 14:35:21     INFO - resource://testing-common/content-task.js line 52 > eval:null:6
[task 2017-08-17T14:35:21.309733Z] 14:35:21     INFO - resource://testing-common/content-task.js line 52 > eval:null:6
[task 2017-08-17T14:35:21.315595Z] 14:35:21     INFO - resource://testing-common/content-task.js:null:53
[task 2017-08-17T14:35:21.318016Z] 14:35:21     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-08-17T14:35:21.321169Z] 14:35:21     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_keyboard.js | Focus should be set on undefined - null == {} - 
[task 2017-08-17T14:35:21.329904Z] 14:35:21     INFO - Stack trace:
[task 2017-08-17T14:35:21.338006Z] 14:35:21     INFO - resource://testing-common/content-task.js line 52 > eval:null:10
[task 2017-08-17T14:35:21.340186Z] 14:35:21     INFO - resource://testing-common/content-task.js:null:53
[task 2017-08-17T14:35:21.343625Z] 14:35:21     INFO - Pressing VK_UP to select onboarding-tour-default-browser and have focus on onboarding-tour-default-browser
Flags: needinfo?(yzenevich)
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70e882c76ef
improve semantics and keyboard accessibility of tour tabs UI in onboarding overlay. r=mossop, gasolin
Rebased and updated tests
Flags: needinfo?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/c70e882c76ef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8893385 [details] [diff] [review]
1377298 tab semantics and keyboard navigation v4


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?]: No
[Needs manual test from QE? If yes, steps to reproduce]:
* When dialog is open, user can use tab, shift tab, arrow up and down to navigate between onboarding tour items.
* Focus should clearly be visible for currently selected tour list item (keyboard only)
* If tour item selection is done with the mouse, focus styling should never be shown, or if there was an existing one, it should be dismissed.
[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]: Added new string onboarding.complete=Complete
Attachment #8893385 - Flags: approval-mozilla-beta?
Do we have particular onboarding planned for 56 release?
Flags: needinfo?(yzenevich)
Just found it in Trello. I'll follow up to make sure we have QA plans. Never mind!
Flags: needinfo?(yzenevich)
Comment on attachment 8893385 [details] [diff] [review]
1377298 tab semantics and keyboard navigation v4

Fix accessibility for onboarding/photon plans in 56. This should land for beta 5 or 6.
Attachment #8893385 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Yura Zenevich [:yzen] from comment #38)
> [String changes made/needed]: Added new string onboarding.complete=Complete

This breaks string freeze, does it not? Has anybody on the l10n side of things reviewed this?
Flags: needinfo?(yzenevich)
Also, this is going to require a rebased patch for Beta once that gets sorted out.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> (In reply to Yura Zenevich [:yzen] from comment #38)
> > [String changes made/needed]: Added new string onboarding.complete=Complete
> 
> This breaks string freeze, does it not? Has anybody on the l10n side of
> things reviewed this?

Nope, and not exactly happy about breaking string freeze mid cycle.

This seems to fix a lot more than just the string, is there any chance to have a patch for Beta only that doesn't touch onboarding.properties? It seems doable, changing

completedText.setAttribute("aria-label", this._bundle.GetStringFromName("onboarding.complete"));

to

completedText.setAttribute("aria-label", "Complete");
(In reply to Francesco Lodolo [:flod] from comment #44)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> > (In reply to Yura Zenevich [:yzen] from comment #38)
> > > [String changes made/needed]: Added new string onboarding.complete=Complete
> > 
> > This breaks string freeze, does it not? Has anybody on the l10n side of
> > things reviewed this?
> 
> Nope, and not exactly happy about breaking string freeze mid cycle.
> 
> This seems to fix a lot more than just the string, is there any chance to
> have a patch for Beta only that doesn't touch onboarding.properties? It
> seems doable, changing
> 
> completedText.setAttribute("aria-label",
> this._bundle.GetStringFromName("onboarding.complete"));
> 
> to
> 
> completedText.setAttribute("aria-label", "Complete");

I'm happy to do that for Beta. I'll take care of the patch, and sorry for not making it in time for the string freeze.
Flags: needinfo?(yzenevich)
I reproduced this issue using Fx 57.0a1 20170815100349, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 57.0a1, build ID: , on Windows 10 x64, macOS X 10.12 and Ubuntu 14.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
Depends on: 1394730
Depends on: 1394731
I can confirm this issue is fixed on Fx 56.0b8 as well, I verified using Windows 10 x64, mac OS X 10.12.6 and Ubuntu 14.04 LTS.

Cheers!
thanks! \o/
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.