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)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 57
People
(Reporter: yzen, Assigned: yzen)
References
Details
(Keywords: access, Whiteboard: [photon-onboarding])
Attachments
(3 files, 4 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mossop
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•7 years ago
|
Blocks: photon-onboarding-accessibility
Comment 1•7 years ago
|
||
according to the discussion with Photon onboarding PM/UX, change to P3
Priority: -- → P3
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
* 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)
Assignee | ||
Comment 3•7 years ago
|
||
Note: this bug depends on the patch from bug 1377285 to properly apply for CSS changes.
Depends on: 1377285
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8890047 -
Flags: ui-review?(mverdi)
Assignee | ||
Comment 6•7 years ago
|
||
(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 7•7 years ago
|
||
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+
Updated•7 years ago
|
Target Milestone: --- → Firefox 57
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
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+
Assignee | ||
Comment 16•7 years ago
|
||
All comments addressed.
Attachment #8893048 -
Attachment is obsolete: true
Attachment #8893048 -
Flags: review?(dtownsend)
Attachment #8893385 -
Flags: review?(dtownsend)
Updated•7 years ago
|
Attachment #8893385 -
Flags: review?(dtownsend) → review+
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
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
Assignee | ||
Comment 20•7 years ago
|
||
This one dis not cause eslint failure so landing separately.
Flags: needinfo?(yzenevich)
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
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
Assignee | ||
Comment 23•7 years ago
|
||
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)
Comment 24•7 years ago
|
||
(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)
Assignee | ||
Comment 25•7 years ago
|
||
(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)
Comment 26•7 years ago
|
||
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.
Assignee | ||
Comment 27•7 years ago
|
||
(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.
Assignee | ||
Comment 28•7 years ago
|
||
ok looks like disabling for debug only solves the issues:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=765ca142b84418ef83e13fc8402bfe7c2ac5ac70
Comment 29•7 years ago
|
||
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
Comment 30•7 years ago
|
||
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)
Comment 31•7 years ago
|
||
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
Comment 32•7 years ago
|
||
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
Assignee | ||
Comment 33•7 years ago
|
||
(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)
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
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
Comment 37•7 years ago
|
||
bugherder |
Assignee | ||
Comment 38•7 years ago
|
||
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?
status-firefox56:
--- → affected
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+
Comment 42•7 years ago
|
||
(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)
Comment 43•7 years ago
|
||
Also, this is going to require a rebased patch for Beta once that gets sorted out.
Comment 44•7 years ago
|
||
(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");
Assignee | ||
Comment 45•7 years ago
|
||
(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)
Comment 46•7 years ago
|
||
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
Comment 47•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 48•7 years ago
|
||
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!
Comment 49•7 years ago
|
||
thanks! \o/
Comment 50•7 years ago
|
||
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.
status-firefox58:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•