1377273 - [a11y] New Tab onboarding icon is not accessible.

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

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

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

Comment 1

[Francis Lee [:frlee]]

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

Comment 2

[[e-mail]]

Attachment: Bug 1377273 - [a11y] Make onboarding icon accessible;

Review commit: https://reviewboard.mozilla.org/r/156906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156906/

Comment 3

[[e-mail]]

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 4

[[e-mail]]

Comment on attachment 8886105 [details]
Bug 1377273 - [a11y] Make onboarding icon accessible;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156906/diff/1-2/

Comment 5

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

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 6

[[e-mail]]

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 7

[[e-mail]]

Comment on attachment 8886105 [details]
Bug 1377273 - [a11y] Make onboarding icon accessible;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156906/diff/2-3/

Comment 8

[[e-mail]]

Comment on attachment 8886105 [details]
Bug 1377273 - [a11y] Make onboarding icon accessible;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156906/diff/3-4/

Comment 9

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

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 10

[[e-mail]]

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 11

[[e-mail]]

Comment on attachment 8886105 [details]
Bug 1377273 - [a11y] Make onboarding icon accessible;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156906/diff/4-5/

Comment 12

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

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;
```

Comment 13

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

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

Comment 14

[Fischer [:Fischer]]

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

[Dave Townsend [:mossop]]

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.

Comment 16

[[e-mail]]

Comment on attachment 8886105 [details]
Bug 1377273 - [a11y] Make onboarding icon accessible;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156906/diff/5-6/

Comment 17

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

Attachment: 1377273 a11y proposal

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 18

[[e-mail]]

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 19

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

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 20

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

Attachment: 1377273 a11y v2

Comment 21

[Dave Townsend [:mossop]]

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

Comment 22

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

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

Comment 23

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

Attachment: 1377273 a11y v3

updated

Comment 24

[[e-mail]]

Attachment: icon with a11y

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.

Comment 25

[Verdi [:verdi] Best to slack me]

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

[[e-mail]]

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 27

[Fischer [:Fischer]]

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.

Comment 28

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

Attachment: 1377273 a11y v4

Comments should be addressed now

Comment 29

[[e-mail]]

Comment on attachment 8893052 [details] [diff] [review]
1377273 a11y v4

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

looks good, thanks!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1ca58ad88e
added focus styling for onboarding overlay button. r=mossop, gasolin

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f1ca58ad88e

Comment 32

[Abe - QA (:Abe_LV)]

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

Comment 33

[[e-mail]]

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

Comment 34

[Abe - QA (:Abe_LV)]

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

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

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 36

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

Comment on attachment 8893052 [details] [diff] [review]
1377273 a11y v4

Please uplift to beta - should improve a11y for onboarding for 56.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/db1c6a7e5ec6

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

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

posted try with rebased commits in bug 1377300

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/29e7e0cd16bf

Comment 41

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

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!

Comment 42

[Justin [:JW_SoftvisionQA]]

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.