There are several things that need to be addressed for it to be accessible: * It must be included in the tab order for the new tab. Since it is the first (topmost/left in LTR) control on the page it should likely be the first one that the user tabs to. * Currently the icon is just a DIV. Semantically though, it is a button that triggers the dialog so it must be BUTTON in the markup. * The button does not have a label either. It must be added and be internationalized (e.g. alt text). * It should also implement "aria-haspopup" to indicate that it has a popup context [1]. 1. https://www.w3.org/TR/wai-aria/states_and_properties#aria-haspopup

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

Hi Yura, the patch * referenced https://www.nczonline.net/blog/2013/04/01/making-accessible-icon-buttons/ and use <input type="image"> which should been treat as the <button> in screen reader * move fox icon as the first body element so it will be first selected * add localizationable alt text and "aria-haspopup" * allow navigate with tab and use space to toggle the onboarding overlay To test onboarding, you can create a new profile and the onboarding fox icon will be shown.

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; https://reviewboard.mozilla.org/r/156906/#review162032 ::: browser/extensions/onboarding/content/onboarding.js:234 (Diff revision 2) > this._tourItems = []; > this._tourPages = []; > this._tours = []; > + this._tourType = Services.prefs.getStringPref("browser.onboarding.tour-type", "update"); > > - let tourIds = this._getTourIDList(Services.prefs.getStringPref("browser.onboarding.tour-type", "update")); > + let tourIds = this._getTourIDList(this._tourType); Not sure if this change is required for the bug. ::: browser/extensions/onboarding/content/onboarding.js:694 (Diff revision 2) > return div; > } > > _renderOverlayIcon() { > - let img = this._window.document.createElement("div"); > - img.id = "onboarding-overlay-icon"; > + let icon = this._window.document.createElement("input"); > + icon.type = "image"; From what I can tell, input type image is used for Submit buttons only. I think this is somewhat incorrect to use it since we are adding all the form action related overhead. We are not actually making a form action when the button is pressed. I would rather use a plain input type button or better (more styleable) button element. ::: browser/extensions/onboarding/content/onboarding.js:699 (Diff revision 2) > - img.id = "onboarding-overlay-icon"; > - return img; > + icon.type = "image"; > + icon.src = "resource://onboarding/img/overlay-icon.svg"; > + icon.id = "onboarding-overlay-icon"; > + let welcomeStringId = this._tourType === "new" ? "onboarding.overlay-icon-tool-tip" : "onboarding.overlay-icon-update-tool-tip"; > + icon.alt = this._bundle.formatStringFromName(welcomeStringId, [BRAND_SHORT_NAME], 1); > + icon.ariaHaspopup = true; nice!

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; https://reviewboard.mozilla.org/r/156906/#review162342 ::: browser/extensions/onboarding/content/onboarding.js:234 (Diff revision 2) > this._tourItems = []; > this._tourPages = []; > this._tours = []; > + this._tourType = Services.prefs.getStringPref("browser.onboarding.tour-type", "update"); > > - let tourIds = this._getTourIDList(Services.prefs.getStringPref("browser.onboarding.tour-type", "update")); > + let tourIds = this._getTourIDList(this._tourType); it's not directly related to the bug, but the refactor helps we reuse the code ::: browser/extensions/onboarding/content/onboarding.js:694 (Diff revision 2) > return div; > } > > _renderOverlayIcon() { > - let img = this._window.document.createElement("div"); > - img.id = "onboarding-overlay-icon"; > + let icon = this._window.document.createElement("input"); > + icon.type = "image"; thanks for clarify, will use plain button instead ::: browser/extensions/onboarding/content/onboarding.js:699 (Diff revision 2) > - img.id = "onboarding-overlay-icon"; > - return img; > + icon.type = "image"; > + icon.src = "resource://onboarding/img/overlay-icon.svg"; > + icon.id = "onboarding-overlay-icon"; > + let welcomeStringId = this._tourType === "new" ? "onboarding.overlay-icon-tool-tip" : "onboarding.overlay-icon-update-tool-tip"; > + icon.alt = this._bundle.formatStringFromName(welcomeStringId, [BRAND_SHORT_NAME], 1); > + icon.ariaHaspopup = true; actually it's not working after double check :/, will use setAttribute instead

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; https://reviewboard.mozilla.org/r/156906/#review162430 ::: browser/extensions/onboarding/content/onboarding.css (Diff revision 4) > > #onboarding-overlay-icon { > - width: 36px; > - height: 29px; > position: absolute; > - cursor: pointer; I think leaving this will indicate it's clickable better, do you have other considerations? ::: browser/extensions/onboarding/content/onboarding.js:287 (Diff revision 4) > this.uiInitialized = true; > > this._overlayIcon = this._renderOverlayIcon(); > this._overlayIcon.addEventListener("click", this); > - this._window.document.body.appendChild(this._overlayIcon); > + this._window.document.body.insertBefore(this._overlayIcon, > + this._window.document.body.firstChild); This change makes the icon unclickable because it is covered by newtab-vertical-margin on newtab page. We may need to do something like adjusting z-index or pointer-events.

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; https://reviewboard.mozilla.org/r/156906/#review162430 > I think leaving this will indicate it's clickable better, do you have other considerations? Thanks for catching that! I was use `input type="image"`, which has point curor hover by default, but I forgot to add it back with `button`

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; https://reviewboard.mozilla.org/r/156906/#review162882 More in-depth review, I added a simpler solution that hopefully does the same thing. Also we can just use title and get a tooltip for free. aria-label should be our last resource. ::: browser/extensions/onboarding/content/onboarding.css:31 (Diff revision 5) > - height: 29px; > position: absolute; > cursor: pointer; > top: 30px; > offset-inline-start: 30px; > - background: url("img/overlay-icon.svg") no-repeat; > + padding: 0; no need for this ::: browser/extensions/onboarding/content/onboarding.css:33 (Diff revision 5) > cursor: pointer; > top: 30px; > offset-inline-start: 30px; > - background: url("img/overlay-icon.svg") no-repeat; > + padding: 0; > + border: 0; > + background-color: inherit; inherit->transparent ::: browser/extensions/onboarding/content/onboarding.css:34 (Diff revision 5) > top: 30px; > offset-inline-start: 30px; > - background: url("img/overlay-icon.svg") no-repeat; > + padding: 0; > + border: 0; > + background-color: inherit; > + z-index: 20998; to be consistent with customize icon, lets set it to 101 ::: browser/extensions/onboarding/content/onboarding.css:37 (Diff revision 5) > + border: 0; > + background-color: inherit; > + z-index: 20998; > +} > + > +#onboarding-overlay-icon > img { no need for this ::: browser/extensions/onboarding/content/onboarding.css:40 (Diff revision 5) > +} > + > +#onboarding-overlay-icon > img { > + width: 36px; > + height: 29px; > } all in all, consistent with customize gear icon we can do this: ``` #onboarding-overlay-icon { position: absolute; top: 30px; offset-inline-start: 30px; z-index: 101; background-image: -moz-image-rect(url(resource://onboarding/img/overlay-icon.svg), 0, 36, 36, 0); background-size: 29px; height: 29px; width: 36px; background-repeat: no-repeat; background-position: center; background-color: transparent; border: none; outline: 0; } /* to remove dotted firefox border */ #onboarding-overlay-icon::-moz-focus-inner { border: 0; } /* nice overlay for hover and focus that is consistent with the gear icon */ #onboarding-overlay-icon:-moz-any(:hover, :active, :focus, [active]) { background-color: #FFFFFF; border: solid 1px #CCCCCC; border-radius: 2px; } ``` ::: browser/extensions/onboarding/content/onboarding.js:698 (Diff revision 5) > div.querySelector("#onboarding-header").textContent = > this._bundle.formatStringFromName("onboarding.overlay-title", [BRAND_SHORT_NAME], 1); > return div; > } > > _renderOverlayIcon() { We can do this without an img inside a button: ``` let icon = this._window.document.createElement("button"); icon.id = "onboarding-overlay-icon"; let tooltip = this._bundle.formatStringFromName("onboarding.overlay-icon-tool-tip", [BRAND_SHORT_NAME], 1); icon.setAttribute("aria-haspopup", true); icon.title = tooltip; return icon; ```

Oh and another nit: can we rename the word 'icon' in selectors everywhere to 'button' since that's what it actually is.

(In reply to Yura Zenevich [:yzen] from comment #12) > all in all, consistent with customize gear icon we can do this: > ``` > #onboarding-overlay-icon { > position: absolute; > top: 30px; > offset-inline-start: 30px; > z-index: 101; > background-image: > -moz-image-rect(url(resource://onboarding/img/overlay-icon.svg), 0, 36, 36, > 0); > background-size: 29px; > If went for `background-image`, we wouldn't see the little fox image in the high-contrast mode. The bug 1377439 will switch from <img> to <button> for this onboarding fox icon button. (In reply to Yura Zenevich [:yzen] from comment #13) > Oh and another nit: can we rename the word 'icon' in selectors everywhere to 'button' since that's what it actually is. The bug 1377439 could do this btw. After discussing with :gasolin, since the bug 1377439 is having higher priority, we are doing it first. Let the bug 1377439 handle this onboarding fox icon button in the high-contrast mode and let this bug handle the rest of work such as button label and tooltip etc. Set the dependency.

Comment on attachment 8886105 [details] Bug 1377273 - [a11y] Make onboarding icon accessible; Sounds like we're waiting on an updated patch after bug 1377439 here.

Hi Fred, I updated the my proposed changes that work with high contrast what do you think? Changes: * button is now in good tab order * using title instead of aria-label * focus/hover styling

Comment on attachment 8891457 [details] [diff] [review] 1377273 a11y proposal Review of attachment 8891457 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'll obsolete my patch here ::: browser/extensions/onboarding/content/onboarding.css @@ +324,5 @@ > } > > /* Remove default dotted outline around buttons' text */ > +.onboarding-tour-action-button::-moz-focus-inner, > +#onboarding-overlay-button::-moz-focus-inner { adding this will let user can't see any visual effect of selection (dot-line rectangle selection), is it intentional? @@ +329,5 @@ > border: 0; > } > > /* Keyboard focus specific outline */ > +.onboarding-tour-action-button:-moz-any(:hover, :active, :focus, :-moz-focusring) { or do you want to add the blue rectangle icon selection visual? then we can keep the above style but need add icon id here ``` #onboarding-overlay-button:-moz-focusring, ``` ::: browser/extensions/onboarding/content/onboarding.js @@ +800,1 @@ > button.id = "onboarding-overlay-button"; do we need to add `aria-haspopup` here? ``` button.setAttribute("aria-haspopup", true); ```

Comment on attachment 8891457 [details] [diff] [review] 1377273 a11y proposal Review of attachment 8891457 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/onboarding/content/onboarding.css @@ +324,5 @@ > } > > /* Remove default dotted outline around buttons' text */ > +.onboarding-tour-action-button::-moz-focus-inner, > +#onboarding-overlay-button::-moz-focus-inner { Yes we would want a rectangle outline instead of the dotted one. @@ +329,5 @@ > border: 0; > } > > /* Keyboard focus specific outline */ > +.onboarding-tour-action-button:-moz-any(:hover, :active, :focus, :-moz-focusring) { removed this since Michael suggested it should be keyboard only.

Comment on attachment 8892453 [details] [diff] [review] 1377273 a11y v2 Review of attachment 8892453 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine but I'd rather the tests be put in an actual test file rather than running for every test that waits for the onboarding icon to appear

(In reply to Dave Townsend [:mossop] from comment #21) > Comment on attachment 8892453 [details] [diff] [review] > 1377273 a11y v2 > > Review of attachment 8892453 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks fine but I'd rather the tests be put in an actual test file rather > than running for every test that waits for the onboarding icon to appear Sounds good, ill update the patch and wait for Fred's r?

updated

Thanks for update the patch, it works great! Though while hover on the icon, the visual rectangle is not needed and the visible `tooltip` looks not right since we already show the message bubble to user. Let's let verdi decide.

(In reply to Fred Lin [:gasolin] from comment #24) > Though while hover on the icon, the visual rectangle is not needed and the > visible `tooltip` looks not right since we already show the message bubble > to user. > > Let's let verdi decide. I agree, for mouse users we shouldn't show the rectangle on hover or click and the tooltip is not needed.

Comment on attachment 8892513 [details] [diff] [review] 1377273 a11y v3 Review of attachment 8892513 [details] [diff] [review]: ----------------------------------------------------------------- Based on Verdi's decision, here are some suggest changes ::: browser/extensions/onboarding/content/onboarding.css @@ +34,5 @@ > background: none; > } > > +/* Hover and focus styling */ > +#onboarding-overlay-button:-moz-any(:hover, :active, :focus, :-moz-focusring) { remove `:hover` @@ +57,5 @@ > box-sizing: content-box; > } > > #onboarding-overlay-button::after { > + content: attr(title); no need change here ::: browser/extensions/onboarding/content/onboarding.js @@ +795,5 @@ > let button = this._window.document.createElement("button"); > let tooltipStringId = this._tourType === "new" ? > "onboarding.overlay-icon-tooltip" : "onboarding.overlay-icon-tooltip-updated"; > let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1); > + button.title = tooltip; button.setAttribute("aria-label", tooltip);

Comment on attachment 8892513 [details] [diff] [review] 1377273 a11y v3 Review of attachment 8892513 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/onboarding/test/browser/browser_onboarding_accessibility.js @@ +8,5 @@ > + resetOnboardingDefaultState(); > + > + info("Wait for onboarding overlay loaded"); > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > + await BrowserTestUtils.loadURI(tab.linkedBrowser, ABOUT_HOME_URL); Just a drive-by: There is a `openTab` helper function in the onboarding's head.js. So the above 2 lines could be simplified into `let tab = await openTab(ABOUT_HOME_URL);` p.s1: Inside the `openTab`, it would make sure the browser is really loaded then proceed. That could avoid some intermittent case. p.s2: Could use `await reloadTab(tab)` if needed to reload a tab.

Comments should be addressed now

Comment on attachment 8893052 [details] [diff] [review] 1377273 a11y v4 Review of attachment 8893052 [details] [diff] [review]: ----------------------------------------------------------------- looks good, thanks!

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1ca58ad88e added focus styling for onboarding overlay button. r=mossop, gasolin

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

Abe, First make sure the behavior is the same for mouse user, ex there should not have a highlight when hover the icon with mouse Then, click the search bar and use `tab` key to navigate, * the fox icon should be the first item in content page * There will be a highlight around the icon when navigating with tab key * while highlighted, can use `space` key to open the onboarding overlay dialog

(In reply to Fred Lin [:gasolin] from comment #33) > Abe, > > First make sure the behavior is the same for mouse user, ex there should not > have a highlight when hover the icon with mouse > > > Then, click the search bar and use `tab` key to navigate, > > * the fox icon should be the first item in content page > * There will be a highlight around the icon when navigating with tab key > * while highlighted, can use `space` key to open the onboarding overlay > dialog Thanks for the steps. I verified this as fixed on the latest nightly.

Comment on attachment 8893052 [details] [diff] [review] 1377273 a11y 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?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: See comment 33 [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 on attachment 8893052 [details] [diff] [review] 1377273 a11y v4 Please uplift to beta - should improve a11y for onboarding for 56.

Not sure if it was this or bug 1387057, but browser_onboarding_tours.js started to permafail after they were uplifted to Beta. Backed out. https://hg.mozilla.org/releases/mozilla-beta/rev/c8da3a874d4a https://treeherder.mozilla.org/logviewer.html#?job_id=124750713&repo=mozilla-beta

posted try with rebased commits in bug 1377300

I can confirm this issue is fixed, I reproduced it with Fx 57.0a1 (build ID: 20170802100302) 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!

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.