Bug 1552179 - Turn on eslint-a11y for the rest of the content-src folder (#5054)

* fixed linting errors for CollapsibleSection and ConfirmDialog * fixed a11y linting & structure for ContextMenu; tweaked the scss to maintain styles for the new structure. * a11y linting for DSImage and ErrorBoundary. * tweaked the tests to accommodate the changes to components * fixed a comment * fixed mochitest failures * fixed test failures
2019-05-22 16:47:16 -04:00 · 2019-05-22 16:47:16 -04:00 · ba1c529780
--- a/content-src/components/CollapsibleSection/CollapsibleSection.jsx
+++ b/content-src/components/CollapsibleSection/CollapsibleSection.jsx
@ -17,6 +17,7 @@ export class _CollapsibleSection extends React.PureComponent {
    super(props);
    this.onBodyMount = this.onBodyMount.bind(this);
    this.onHeaderClick = this.onHeaderClick.bind(this);
+    this.onKeyPress = this.onKeyPress.bind(this);
    this.onTransitionEnd = this.onTransitionEnd.bind(this);
    this.enableOrDisableAnimation = this.enableOrDisableAnimation.bind(this);
    this.onMenuButtonClick = this.onMenuButtonClick.bind(this);
@ -91,6 +92,12 @@ export class _CollapsibleSection extends React.PureComponent {
    }));
  }

+  onKeyPress(event) {
+    if (event.key === "Enter" || event.key === " ") {
+      this.onHeaderClick();
+    }
+  }
+
  _getSectionBodyHeight() {
    const div = this.sectionBody;
    if (div.style.display === "none") {
@ -153,11 +160,12 @@ export class _CollapsibleSection extends React.PureComponent {
        <div className="section-top-bar">
          <h3 className="section-title">
            <span className="click-target-container">
-              <span className="click-target" onClick={this.onHeaderClick}>
+            {/* Click-targets that toggle a collapsible section should have an aria-expanded attribute; see bug 1553234 */}
+              <span className="click-target" role="button" tabIndex="0" onKeyPress={this.onKeyPress} onClick={this.onHeaderClick}>
                {this.renderIcon()}
                {getFormattedMessage(title)}
              </span>
-              <span className="click-target" onClick={this.onHeaderClick}>
+              <span className="click-target" role="button" tabIndex="0" onKeyPress={this.onKeyPress} onClick={this.onHeaderClick}>
                {isCollapsible && <span className={`collapsible-arrow icon ${collapsed ? "icon-arrowhead-forward-small" : "icon-arrowhead-down-small"}`} />}
              </span>
              <span className="learn-more-link-wrapper">
--- a/content-src/components/ConfirmDialog/ConfirmDialog.jsx
+++ b/content-src/components/ConfirmDialog/ConfirmDialog.jsx
@ -56,7 +56,7 @@ export class _ConfirmDialog extends React.PureComponent {
    }

    return (<div className="confirmation-dialog">
-      <div className="modal-overlay" onClick={this._handleCancelBtn} />
+      <div className="modal-overlay" onClick={this._handleCancelBtn} role="presentation" />
      <div className="modal">
        <section className="modal-message">
          {this.props.data.icon && <span className={`icon icon-spacer icon-${this.props.data.icon}`} />}
--- a/content-src/components/ContextMenu/ContextMenu.jsx
+++ b/content-src/components/ContextMenu/ContextMenu.jsx
@ -37,11 +37,11 @@ export class ContextMenu extends React.PureComponent {
  }

  render() {
-    return (<span className="context-menu" onClick={this.onClick}>
-      <ul role="menu" className="context-menu-list">
+    return (<span role="menu" className="context-menu" onClick={this.onClick} onKeyDown={this.onClick} tabIndex="0" >
+      <ul className="context-menu-list">
        {this.props.options.map((option, i) => (option.type === "separator" ?
          (<li key={i} className="separator" />) :
-          (option.type !== "empty" && <ContextMenuItem key={i} option={option} hideContext={this.hideContext} />)
+          (option.type !== "empty" && <ContextMenuItem key={i} option={option} hideContext={this.hideContext} tabIndex="0" />)
        ))}
      </ul>
    </span>);
@ -81,11 +81,11 @@ export class ContextMenuItem extends React.PureComponent {
  render() {
    const {option} = this.props;
    return (
-      <li role="menuitem" className="context-menu-item">
-        <a onClick={this.onClick} onKeyDown={this.onKeyDown} tabIndex="0" className={option.disabled ? "disabled" : ""}>
+      <li role="menuitem" className="context-menu-item" >
+        <button className={option.disabled ? "disabled" : ""} onClick={this.onClick} onKeyDown={this.onKeyDown} tabIndex="0" >
          {option.icon && <span className={`icon icon-spacer icon-${option.icon}`} />}
          {option.label}
-        </a>
+        </button>
      </li>);
  }
 }
--- a/content-src/components/ContextMenu/_ContextMenu.scss
+++ b/content-src/components/ContextMenu/_ContextMenu.scss
@ -24,13 +24,16 @@
        margin: $context-menu-outer-padding 0;
      }

-      > a {
+      > a,
+      > button {
        align-items: center;
        color: inherit;
        cursor: pointer;
        display: flex;
+        width: 100%;
        line-height: 16px;
        outline: none;
+        border: 0;
        padding: $context-menu-item-padding;
        white-space: nowrap;

--- a/content-src/components/DiscoveryStreamComponents/DSImage/DSImage.jsx
+++ b/content-src/components/DiscoveryStreamComponents/DSImage/DSImage.jsx
@ -78,13 +78,13 @@ export class DSImage extends React.PureComponent {
            this.state.containerHeight * 2
          );

-          img = (<img crossOrigin="anonymous"
+          img = (<img alt="" crossOrigin="anonymous"
            onError={this.onOptimizedImageError}
            src={source}
            srcSet={`${source2x} 2x`} />);
        }
      } else if (!this.state.nonOptimizedImageFailed) {
-        img = (<img crossOrigin="anonymous"
+        img = (<img alt="" crossOrigin="anonymous"
          onError={this.onNonOptimizedImageError}
          src={this.props.source} />);
      } else {
--- a/content-src/components/ErrorBoundary/ErrorBoundary.jsx
+++ b/content-src/components/ErrorBoundary/ErrorBoundary.jsx
@ -1,3 +1,4 @@
+import {A11yLinkButton} from "content-src/components/A11yLinkButton/A11yLinkButton";
 import {FormattedMessage} from "react-intl";
 import React from "react";

@ -25,7 +26,7 @@ export class ErrorBoundaryFallback extends React.PureComponent {
      className = defaultClass;
    }

-    // href="#" to force normal link styling stuff (eg cursor on hover)
+    // "A11yLinkButton" to force normal link styling stuff (eg cursor on hover)
    return (
      <div className={className}>
        <div>
@ -34,11 +35,11 @@ export class ErrorBoundaryFallback extends React.PureComponent {
            id="error_fallback_default_info" />
        </div>
        <span>
-          <a href="#" className="reload-button" onClick={this.onClick}>
+          <A11yLinkButton className="reload-button" onClick={this.onClick}>
            <FormattedMessage
              defaultMessage="Refresh page to try again."
              id="error_fallback_default_refresh_suggestion" />
-          </a>
+          </A11yLinkButton>
        </span>
      </div>
    );
--- a/package.json
+++ b/package.json
@ -132,7 +132,7 @@
    "debugcoverage": "open logs/coverage/index.html",
    "lint": "npm-run-all lint:*",
    "lint:eslint": "esw --ext=.js,.jsm,.json,.jsx .",
-    "lint:jsx-a11y": "esw --config=.eslintrc.jsx-a11y.js --ext=.jsx content-src/asrouter content-src/components/ASRouterAdmin content-src/components/TopSites",
+    "lint:jsx-a11y": "esw --config=.eslintrc.jsx-a11y.js --ext=.jsx content-src/",
    "lint:sasslint": "sass-lint -v -q",
    "strings-import": "node ./bin/strings-import.js",
    "test": "npm run testmc",
--- a/test/browser/browser_highlights_section.js
+++ b/test/browser/browser_highlights_section.js
@ -61,7 +61,7 @@ test_highlights(
    const contextMenu = content.document.querySelector(".card-outer .context-menu");
    ok(contextMenu && !contextMenu.hidden, "Should find a visible context menu");

-    const removeBookmarkBtn = contextMenu.querySelector("a .icon-bookmark-added");
+    const removeBookmarkBtn = contextMenu.querySelector("button .icon-bookmark-added");
    removeBookmarkBtn.click();

    await ContentTaskUtils.waitForCondition(() => content.document.querySelectorAll(".card-context-icon.icon-bookmark-added").length === 0,
--- a/test/browser/browser_topsites_contextMenu_options.js
+++ b/test/browser/browser_topsites_contextMenu_options.js
@ -13,6 +13,7 @@ test_newtab({
      "Topsite tippytop icon not found");

    const contextMenuItems = content.openContextMenuAndGetOptions(siteSelector).map(v => v.textContent);
+
    Assert.equal(contextMenuItems.length, 5, "Number of options is correct");

    const expectedItemsText = ["Pin", "Edit", "Open in a New Window", "Open in a New Private Window", "Dismiss"];
@ -41,7 +42,7 @@ test_newtab({
    const contextMenuItems = content.openContextMenuAndGetOptions(siteSelector);
    Assert.equal(contextMenuItems[4].textContent, "Dismiss", "'Dismiss' is the 5th item in the context menu list");

-    contextMenuItems[4].querySelector("a").click();
+    contextMenuItems[4].querySelector("button").click();

    // Wait for the topsite to be dismissed and the second one to replace it
    await ContentTaskUtils.waitForCondition(() => content.document.querySelector(siteSelector).getAttribute("href") === secondTopSite,
@ -65,7 +66,7 @@ test_newtab({
    is(contextMenuItems.length, 2, "Search TopSites should only have Unpin and Dismiss");

    // Unpin
-    contextMenuItems[0].querySelector("a").click();
+    contextMenuItems[0].querySelector("button").click();

    await ContentTaskUtils.waitForCondition(() => content.document.querySelectorAll(siteSelector).length === 1,
      "1 search topsite displayed after we unpin the other one");
--- a/test/browser/browser_topsites_section.js
+++ b/test/browser/browser_topsites_section.js
@ -9,7 +9,7 @@ test_newtab(
    let contextMenu = content.document.querySelector(".top-sites .section-top-bar .context-menu");
    ok(contextMenu, "Should find a visible topsite context menu");

-    const topsitesAddBtn = content.document.querySelector(".top-sites .context-menu-item a");
+    const topsitesAddBtn = content.document.querySelector(".top-sites .context-menu-item button");
    topsitesAddBtn.click();

    let found = content.document.querySelector(".topsite-form");
@ -39,7 +39,7 @@ test_newtab({
    let contextMenu = topsiteEl.querySelector(".top-sites-list .context-menu");
    ok(contextMenu, "Should find a topsite context menu");

-    const pinUnpinTopsiteBtn = contextMenu.querySelector(".top-sites .context-menu-item a");
+    const pinUnpinTopsiteBtn = contextMenu.querySelector(".top-sites .context-menu-item button");
    // Pin the topsite.
    pinUnpinTopsiteBtn.click();

@ -54,7 +54,7 @@ test_newtab({
    topsiteContextBtn = topsiteEl.querySelector(".context-menu-button");
    ok(topsiteContextBtn, "Should find a context menu button");
    topsiteContextBtn.click();
-    topsiteEl.querySelector(".context-menu-item a").click();
+    topsiteEl.querySelector(".context-menu-item button").click();

    // Need to wait for unpin action.
    await ContentTaskUtils.waitForCondition(() => !topsiteEl.querySelector(".icon-pin-small"),
@ -75,7 +75,7 @@ test_newtab({
    let contextMenu = content.document.querySelector(".top-sites .section-top-bar .context-menu");
    ok(contextMenu, "Should find a visible topsite context menu");

-    const topsitesAddBtn = content.document.querySelector(".top-sites .context-menu-item a");
+    const topsitesAddBtn = content.document.querySelector(".top-sites .context-menu-item button");
    topsitesAddBtn.click();

    let found = content.document.querySelector(".modal-overlay");
@ -112,7 +112,7 @@ test_newtab({

    let contextMen = content.document.querySelector(".top-sites-list .context-menu");

-    const dismissBtn = contextMen.querySelector(".top-sites .context-menu-item a .icon-dismiss");
+    const dismissBtn = contextMen.querySelector(".top-sites .context-menu-item button .icon-dismiss");
    dismissBtn.click();

    // Wait for Topsite to be removed
--- a/test/unit/content-src/components/ContextMenu.test.jsx
+++ b/test/unit/content-src/components/ContextMenu.test.jsx
@ -38,7 +38,7 @@ describe("<ContextMenu>", () => {
    const onUpdate = sinon.spy();
    const onClick = sinon.spy();
    const wrapper = mount(<ContextMenu {...DEFAULT_PROPS} onUpdate={onUpdate} options={[{label: "item1", onClick}]} />);
-    wrapper.find(".context-menu-item a").simulate("click");
+    wrapper.find(".context-menu-item button").simulate("click");
    assert.calledOnce(onUpdate);
    assert.calledOnce(onClick);
  });
@ -50,6 +50,6 @@ describe("<ContextMenu>", () => {
  it("should add disabled className to any disabled options", () => {
    const options = [{label: "item1", icon: "icon1", disabled: true}, {type: "separator"}];
    const wrapper = mount(<ContextMenu {...DEFAULT_PROPS} options={options} />);
-    assert.lengthOf(wrapper.find(".context-menu-item a.disabled"), 1);
+    assert.lengthOf(wrapper.find(".context-menu-item button.disabled"), 1);
  });
 });
--- a/test/unit/content-src/components/ErrorBoundary.test.jsx
+++ b/test/unit/content-src/components/ErrorBoundary.test.jsx
@ -75,10 +75,10 @@ describe("ErrorBoundaryFallback", () => {
    assert.calledWithExactly(fakeWindow.location.reload, true);
  });

-  it("should render .reload-button as an <a> element with an href attr", () => {
+  it("should render .reload-button as an <A11yLinkButton>", () => {
    const wrapper = shallow(<ErrorBoundaryFallback />);

-    assert.lengthOf(wrapper.find(".reload-button[href]"), 1);
+    assert.lengthOf(wrapper.find("A11yLinkButton.reload-button"), 1);
  });

  it("should render error_fallback_default_refresh_suggestion FormattedMessage",
--- a/yamscripts.yml
+++ b/yamscripts.yml
@ -64,7 +64,7 @@ scripts:
 # lint: Run eslint and sass-lint
  lint:
    eslint: esw --ext=.js,.jsm,.json,.jsx .
-    jsx-a11y: esw --config=.eslintrc.jsx-a11y.js --ext=.jsx content-src/asrouter content-src/components/ASRouterAdmin content-src/components/TopSites
+    jsx-a11y: esw --config=.eslintrc.jsx-a11y.js --ext=.jsx content-src/
    sasslint: sass-lint -v -q

 # strings-import: Replace local strings with those from l10n-central