Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Accessibility Panel #87

Merged

Conversation

philschatz
Copy link
Contributor

@philschatz philschatz commented Sep 27, 2016

Thanks for adding tools to help make electron apps more accessible!

I stumbled upon this while fixing accessibility-developer-tools#291 and as a long-time Atom user/hacker, thought I'd leave this repository a little better off than how I found it.

accessibility-link

TODO

  • make the results elements clickable so they open in the Element Inspector
  • use the now-working accessibility-developer-tools module instead of using vendor/axs.js
    • this is non-trivial because accessibility.js uses a private function named axs.utils.getQuerySelectorText(element)

evt.preventDefault()
chrome.devtools.inspectedWindow.eval(
"inspect($$(\"" + this.path.replace('"', '\\"') + "\")[0])",
function(result, isException) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a internal helper that is promise-based instead of callback-based you might want to use here.

https://github.com/electron/devtron/blob/master/lib/eval.js#L4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed that 😊 . Fixed

@kevinsawicki
Copy link
Contributor

make the results elements clickable so they open in the Element Inspector

This is great 👍

use the now-working accessibility-developer-tools module instead of using vendor/axs.js

Is the path forward here to open up a pull request upstream to make this a public API?

@kevinsawicki kevinsawicki self-assigned this Sep 27, 2016
@philschatz
Copy link
Contributor Author

philschatz commented Sep 28, 2016

Thanks!

Yes, that might work but... the method generates selectors for an element and seems to only be used internally to generate error messages.

Before creating an Issue, I thought I'd solicit feedback on an approach that does not require exposing axs.utils.getQuerySelectorText(element).

I just pushed a commit (7f1d782) that is more accurate than relying on a generated CSS selector. It is also similar to how react-devtools solved the same problem.

If that strategy seems convoluted or more difficult to maintain I submitted can submit an Issue to GoogleChrome/accessibility-developer-tools#305

Getting access to the package in the browser

I was unable to get the electron app access to require('accessibility-developer-tools') so instead of the code looking like this:

// This code does not work

exports.audit = () => {
  return Eval.execute(function () {
    // This code is serialized and runs in the browser. 
    // It currently relies on the content_scripts in manifest.json to get access to axs via the window.__devtron.axs global
    const axs = require('accessibility-developer-tools')
    const config = new axs.AuditConfiguration({showUnsupportedRulesWarning: false})

... I added out/browserGlobals.js which is built using browserify and defined in the manifest.json (see ddaa402)

There is probably a better way that I missed, but I could not figure it out.

@@ -0,0 +1,8 @@
// This defines globals that will be used in the browser context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the filename here should be browser-globals.js to match the naming convention of the other files in this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, 😊 . Fixed!

@kevinsawicki
Copy link
Contributor

Really cool, thanks for this 👍

@kevinsawicki kevinsawicki merged commit 3873357 into electron-userland:master Oct 7, 2016
@kevinsawicki
Copy link
Contributor

Published in 1.4.0 🎉

@zeke
Copy link
Member

zeke commented Oct 11, 2016

This seems kinda blog-worthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants