mozilla / send

Simple, private file sharing from the makers of Firefox
https://send.firefox.com
Mozilla Public License 2.0
13.23k stars 1.53k forks source link

A few HTML and accessibility/usability errors on send.firefox.com #139

Closed pdehaan closed 6 years ago

pdehaan commented 7 years ago

Ref: https://github.com/mozilla/send/issues/139#issuecomment-320333492

pdehaan commented 7 years ago

Just spotted this tool on Twitter and it looks pretty interesting. Possibly a replacement for the https://github.com/GoogleChrome/lighthouse tool.

We can close this or break it down into more granular issues if needed.


# Install the `sonar` dependency locally (so we don't need to install globally).
> npm i @sonarwhal/sonar -D

# Create our .sonarrc config file using the CLI wizard.
> $(npm bin)/sonar --init

After a bit of trial-and-error, I found this to be a decent config:

{
  "browserslist": "",
  "collector": {
    "name": "cdp",
    "options": {}
  },
  "formatter": "stylish",
  "ignoredUrls": {
    "code.cdn.mozilla.net": ["*"]
  },
  "rules": {
    "axe": "error",
    "content-type": "error",
    "disallowed-headers": "error",
    "disown-opener": "error",
    "highest-available-document-mode": "error",
    "manifest-exists": "off",
    "manifest-file-extension": "error",
    "manifest-is-valid": "error",
    "meta-charset-utf-8": "error",
    "no-friendly-error-pages": "error",
    "no-html-only-headers": "off",
    "no-protocol-relative-urls": "error",
    "ssllabs": "error",
    "x-content-type-options": "error"
  },
  "rulesTimeout": 120000
}

Next, you can run the CLI tool against a remote URL using something like this:

> $(npm bin)/sonar https://fileshare.stage.mozaws.net

After a few seconds-minutes, you should get something like the following:

✖ Finishing...

https://fileshare.stage.mozaws.net/bundle.js
line 3  col 3  Error  'content-type' header should have 'charset=utf-8'  content-type
✖ Found 1 error and 0 warnings

https://fileshare.stage.mozaws.net/resources/upload.svg
line 15  col 30  Error  'content-type' header should have 'charset=utf-8'  content-type
✖ Found 1 error and 0 warnings

https://fileshare.stage.mozaws.net/
                 Error  'x-ua-compatible' header was not specified    highest-available-document-mode
                 Error  No charset meta tag was specified             meta-charset-utf-8
line 1   col 16  Error  <html> element must have a lang attribute     axe
line 15  col 9   Error  id attribute value must be unique             axe
line 25  col 15  Error  Elements must have sufficient color contrast  axe
✖ Found 5 errors and 0 warnings

✖ Found a total of 7 errors and 0 warnings
Exit code: 1

I'm still playing with the tool, but it looks pretty cool. Possibly a bit too slow and overkill for running on Circle-CI or something per each commit though. But I kind of like how it found a few a11y issues and other missing HTML best practice bits. Plus, it runs against generated HTML, so it won't give us the same Handlebars in HTML linting issues we saw w/ htmllint (which may be a pro and con).


Nothing too dramatic showing up in a11y results:

> npm i a11y -D
> $(npm bin)/a11y https://fileshare.stage.mozaws.net/

  ✖ Controls and media elements should have labels

  #link

  ✔ This element does not support ARIA roles, states and properties
  ✔ Any ID referred to via an IDREF must be unique in the DOM

I think it's complaining about this DOM element <input id="link" type="url" value="" readonly/> and it's lack of an associated <label> tag.


I'm not aware of any other ESLint-y a11y linters, apart from https://www.npmjs.com/package/eslint-plugin-jsx-a11y. Although that one (while super awesome) relies on JSX, which may mean some front end rewriting and possibly converting to JSX/React.

pdehaan commented 7 years ago

Most of these should be fixed with the new design, but I can re-run the scanner after the new design lands and make sure we pass a11y and basic HTML linting.

pdehaan commented 6 years ago

Latest results:

npm run sonar https://send.dev.mozaws.net/

> sonar-test@1.0.0 sonar /Users/pdehaan/dev/tmp/del/sonar-test
> sonar "https://send.dev.mozaws.net/"

✖ Finishing...
https://send.dev.mozaws.net/
                  Error  'x-ua-compatible' header was not specified                                                                                                                highest-available-document-mode
line 14   col 3   Error  Bad value “localization” for attribute “rel” on element “link”: The string “localization” is not a registered keyword.                                    html-checker
line 14   col 3   Error  Bad value “/locales/{locale}/send.ftl” for attribute “href” on element “link”: Illegal character in path segment: “{” is not allowed.                     html-checker
line 21   col 7   Error  Text of buttons and links should not be repeated in the image alternative                                                                                 axe
line 25   col 19  Error  '<a href="https://testpilot.firefox.com" target="_blank">Firefox Test Pilot</a>' is missing 'rel' values 'noopener', 'noreferrer'                         disown-opener
line 47   col 26  Error  An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.  html-checker
line 49   col 7   Error  Elements must have sufficient color contrast                                                                                                              axe
line 62   col 46  Error  The “width” attribute on the “th” element is obsolete. Use CSS instead.                                                                                   html-checker
line 63   col 60  Error  The “width” attribute on the “th” element is obsolete. Use CSS instead.                                                                                   html-checker
line 64   col 60  Error  The “width” attribute on the “th” element is obsolete. Use CSS instead.                                                                                   html-checker
line 65   col 62  Error  The “width” attribute on the “th” element is obsolete. Use CSS instead.                                                                                   html-checker
line 75   col 1   Error  Bad value “true” for attribute “hidden” on element “div”.                                                                                                 html-checker
line 93   col 1   Error  Bad value “true” for attribute “hidden” on element “div”.                                                                                                 html-checker
line 106  col 1   Error  Bad value “true” for attribute “hidden” on element “div”.                                                                                                 html-checker
line 109  col 3   Error  An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.  html-checker
line 112  col 7   Error  Links must have discernible text                                                                                                                          axe
line 112  col 17  Error  '<a href="https://www.mozilla.org" target="_blank … illa-logo" src="/resources/mozilla-logo.svg"></a>' is missing 'rel' values 'noopener', 'noreferrer'   disown-opener
line 112  col 57  Error  Images must have alternate text                                                                                                                           axe
line 113  col 17  Error  '<a href="https://www.mozilla.org/about/legal" da … 0n-id="footerLinkLegal" target="_blank">Legal</a>' is missing 'rel' values 'noopener', 'noreferrer'   disown-opener
line 114  col 17  Error  '<a href="https://testpilot.firefox.com/about" da … erLinkAbout" target="_blank">About Test Pilot</a>' is missing 'rel' values 'noopener', 'noreferrer'   disown-opener
line 116  col 57  Error  An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.  html-checker
line 117  col 17  Error  '<a href="https://www.mozilla.org/privacy/website … d="footerLinkCookies" target="_blank">Cookies</a>' is missing 'rel' values 'noopener', 'noreferrer'   disown-opener
line 120  col 7   Error  Links must have discernible text                                                                                                                          axe
line 120  col 91  Error  Images must have alternate text                                                                                                                           axe
line 121  col 7   Error  Links must have discernible text                                                                                                                          axe
line 121  col 91  Error  Images must have alternate text                                                                                                                           axe
line 124  col 91  Error  An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.  html-checker
line 125  col 91  Error  An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.  html-checker
✖ Found 28 errors and 0 warnings

✖ Found a total of 28 errors and 0 warnings
Exit code: 1

.sonarrc

{
    "browserslist": "",
    "connector": {
        "name": "cdp",
        "options": {
            "waitFor": 1000
        }
    },
    "formatter": "stylish",
      "ignoredUrls": {
        "code.cdn.mozilla.net": ["*"],
        "analysis-output.telemetry.mozilla.org": ["*"],
        "ssl.google-analytics.com": ["*"]
      },
    "rules": {
        "axe": "error",
        "content-type": "off",
        "disown-opener": "error",
        "highest-available-document-mode": "error",
        "html-checker": "error",
        "manifest-exists": "off",
        "manifest-file-extension": "off",
        "manifest-is-valid": "off",
        "meta-charset-utf-8": "error",
        "no-disallowed-headers": "error",
        "no-friendly-error-pages": "error",
        "no-html-only-headers": "off",
        "no-protocol-relative-urls": "error",
        "ssllabs": "error",
        "x-content-type-options": "error"
    },
    "rulesTimeout": 120000
}
TheAdnan commented 6 years ago

Hi, are these issues still open, I'd like to help :)

vladikoff commented 6 years ago

@TheAdnan Cool! please !

ajomadlabs commented 6 years ago

Hi, Can I also help in fixing these

k0mpreni commented 6 years ago

@pdehaan Can you update your list ? All issues are closed.