Add DomNode renderer #2206

paulirish · 2017-05-10T05:58:58Z

Adding a new node/element renderer for the report. Primarily assisting all the a11y audits right now.

~~I've also added a --a11y CLI flag because 1) it was super easy and 2) it was really handy. But I can revert, no big deal.~~

This also enables node revealing/hovering in devtools. The WIP DevTools-side patch:
https://codereview.chromium.org/2849463002/diff/1/third_party/WebKit/Source/devtools/front_end/audits2/Audits2Panel.js

It looks like this in the standalone report:

patrickhulce · 2017-05-10T16:08:08Z

lighthouse-core/gather/gatherers/accessibility.js

+    const path = [];
+    while (node && node.parentNode) {
+      const index = getNodeIndex(node);
+      path.push([index, node.nodeName]);


is this pavel's special node selector methodology? what would be the downside to augmenting a regular CSS selector with nth-child?

yeah in the protocol docs it says "proprietary format". :)

impl's: backend side,frontend side

downsides of using selectors:

can't reference text nodes, document fragments, or the doctype node

can't pierce shadow dom

patrickhulce · 2017-05-10T16:10:11Z

lighthouse-core/report/v2/renderer/details-renderer.js

+   * @param {!DetailsRenderer.NodeDetailsJSON} item
+   * @return {!Element}
+   */
+  _renderNode(item) {


node is slightly ambiguous at this level, can we make it DOMNode or something? when I just looked at the title of the PR I thought it was a renderer running in "node.js" 😆

patrickhulce · 2017-05-10T16:10:45Z

lighthouse-core/report/v2/renderer/dom.js

+   * @param {string} text
+   * @return {!Node}
+   */
+  createText(text) {


this doesn't seem used?

patrickhulce · 2017-05-10T16:12:26Z

lighthouse-cli/bin.ts

@@ -66,6 +67,8 @@ if (cliFlags.configPath) {
  config = require(cliFlags.configPath);
 } else if (cliFlags.perf) {
  config = perfOnlyConfig;
+} else if (cliFlags.a11y) {


only downside here is that people might think you could do lighthouse --perf --a11y and get both, what if we supported that by nixing the config files and just appending each category that's enabled to onlyCategories of a default extending config

I can pickup that separately though

heh true. ill probably kick this out of the PR. :)

ebidel · 2017-05-10T17:06:53Z

lighthouse-cli/cli-flags.ts

@@ -82,6 +82,7 @@ export function getFlags() {
        'chrome-flags':
            'Custom flags to pass to Chrome (space-delimited). For a full list of flags, see http://peter.sh/experiments/chromium-command-line-switches/.',
        'perf': 'Use a performance-test-only configuration',
+        'a11y': 'Use an accessibility-test-only configuration',


if you keep the flag, please update the readme.

ill probably nuke, but thx!

ebidel · 2017-05-10T17:24:58Z

lighthouse-core/gather/gatherers/accessibility.js

  });
+
+  // Adapated from DevTools' SDK.DOMNode.prototype.path


ebidel · 2017-05-10T17:25:36Z

lighthouse-core/gather/gatherers/accessibility.js

+
+  // Adapated from DevTools' SDK.DOMNode.prototype.path
+  //   https://github.com/ChromeDevTools/devtools-frontend/blob/7a2e162ddefd/front_end/sdk/DOMModel.js#L530-L552
+  // TODO: Doesn't handle frames or shadow roots...


Can you file a bug on this? We have some other "handle shadow root" updates to make in the codebase. We can address them all after io.

I think there's also work ongoing in axe to support shadow dom. @alice may know more.

I don't have any updates here, sorry. I know Marcy was thinking of taking a look at it but she might have gotten swamped with other things.

filed #2243

ebidel · 2017-05-10T17:25:48Z

lighthouse-core/gather/gatherers/accessibility.js

+  //   https://github.com/ChromeDevTools/devtools-frontend/blob/7a2e162ddefd/front_end/sdk/DOMModel.js#L530-L552
+  // TODO: Doesn't handle frames or shadow roots...
+  function getNodePath(node) {
+    /* global Node */


prefer this at the top.

will move to the top top.

look like global and env are file-wide so we couldnt have it be scoped to a function anyway. bummer.

ebidel · 2017-05-10T17:25:55Z

lighthouse-core/gather/gatherers/accessibility.js

+      while (node = node.previousSibling) {
+        // skip empty text nodes
+        if (node.nodeType === Node.TEXT_NODE &&
+            node.textContent.trim().length === 0) continue;


2 space indent

ebidel · 2017-05-10T17:26:10Z

lighthouse-core/gather/gatherers/accessibility.js

+    return path.join(',');
+  }
+
+  function getOuterHTMLSnippet(node) {


paulirish · 2017-05-10T23:40:47Z

I'm getting a ": Object reference chain is too long" error on some pages:

It's due to an object being > 1000 objects deep.

I imagine it's something that happens to the Axe result object on big pages (e.g. https://developers.google.com/web/tools/chrome-devtools/ ) but it's not clear.

cc @robdodson

robdodson · 2017-05-10T23:59:38Z

Hm I'm not able to reproduce this error. Do you have some specific steps I could follow? I tried running axe extension, axe cli, and audits against the link you provided and they all work fine for me.

paulirish · 2017-05-11T00:06:05Z

Hm I'm not able to reproduce this error. Do you have some specific steps I could follow?

I'd say try it with this patch? it's possible the new elemRef option is what's triggering it.

dylanb · 2017-05-11T13:54:13Z

@paulirish I was also not able to reproduce this. I tried using an extension and I tried by simply going to the page, using the console to execute the axe-core 2.2.0 Javascript and then running axe.run - is there some particular environment I need to reproduce this?

DevTools-side patch: https://codereview.chromium.org/2849463002/diff/1/third_party/WebKit/Source/devtools/front_end/audits2/Audits2Panel.js

paulirish · 2017-05-13T07:53:10Z

I'm getting a ": Object reference chain is too long" error on some pages:

I'm pretty sure I managed to squash all these.
I pretty much neuter the return object from axe, in order to verify there are no circular references within it. 4b00b65

thank you guys for testing this and trying to give it a shot.

paulirish · 2017-05-13T07:55:54Z

@dylanb i don't think it'll be a problem for us anymore, but ultimately the challenge was that we wanted JSON.stringify(axeResultObject) to succeed and it was running into circular JSON problems. It doesn't necessarily have to be a requirement that the axeResultObject is serializable, but it might help other consumers of the data.
Cheers!

This reverts commit 6a8b604.

paulirish · 2017-05-15T20:48:27Z

@patrickhulce this is ready for another look right?

patrickhulce · 2017-05-15T21:52:30Z

yep tests passing and ready for review

dylanb · 2017-05-19T18:56:57Z

@paulirish It was in fact a bug, it has been fixed. Upgrade to 2.2.1 to get the fix dequelabs/axe-core#334

paulirish force-pushed the a11ynoderevealing branch from 5ee6497 to 1067183 Compare May 10, 2017 06:15

patrickhulce reviewed May 10, 2017

View reviewed changes

ebidel reviewed May 10, 2017

View reviewed changes

paulirish force-pushed the a11ynoderevealing branch from 1067183 to cc050f5 Compare May 10, 2017 23:18

paulirish changed the title ~~Add Node renderer~~ Add DomNode renderer May 10, 2017

paulirish force-pushed the a11ynoderevealing branch from cc050f5 to 5da3ee6 Compare May 10, 2017 23:43

paulirish mentioned this pull request May 11, 2017

Report: add stub for NodeDetails renderer #2226

Merged

paulirish force-pushed the a11ynoderevealing branch 3 times, most recently from 8883bbd to b81b881 Compare May 13, 2017 07:48

paulirish added 5 commits May 13, 2017 00:50

Prepare to reveal nodes in devtools

39c4652

DevTools-side patch: https://codereview.chromium.org/2849463002/diff/1/third_party/WebKit/Source/devtools/front_end/audits2/Audits2Panel.js

add --a11y cli flag.

6a8b604

node revealing cleanup.

463114c

renderList gets commas

0d8c397

Safer return of axe results

4b00b65

paulirish force-pushed the a11ynoderevealing branch from b81b881 to 4b00b65 Compare May 13, 2017 07:51

patrickhulce added 2 commits May 13, 2017 09:40

Revert "add --a11y cli flag."

464b7b3

This reverts commit 6a8b604.

feedback

d34d018

patrickhulce mentioned this pull request May 13, 2017

Support shadow DOM across all audits #2243

Open

3 tasks

patrickhulce added 2 commits May 13, 2017 09:51

typo fix

f579714

remove comma from list styles

9e94a80

paulirish merged commit b86ee4f into master May 15, 2017

paulirish deleted the a11ynoderevealing branch May 15, 2017 22:12

patrickhulce mentioned this pull request May 18, 2017

[A11y Audit] Audit Error: ... Protocol error (Runtime.evaluate): Object reference chain is too long #2265

Closed

robdodson added a commit that referenced this pull request May 22, 2017

Rev axe to address #2206

1f7f240

robdodson mentioned this pull request May 22, 2017

Rev axe to address #2206 #2335

Merged

paulirish pushed a commit that referenced this pull request May 22, 2017

Rev axe to address #2206 (#2335)

3716658

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DomNode renderer #2206

Add DomNode renderer #2206

paulirish commented May 10, 2017 •

edited

Loading

patrickhulce May 10, 2017

paulirish May 10, 2017

patrickhulce May 10, 2017

patrickhulce May 10, 2017

paulirish May 10, 2017

patrickhulce May 10, 2017

paulirish May 10, 2017

ebidel May 10, 2017

paulirish May 10, 2017

ebidel May 10, 2017

ebidel May 10, 2017

robdodson May 10, 2017

alice May 11, 2017

patrickhulce May 13, 2017

ebidel May 10, 2017

paulirish May 10, 2017

ebidel May 10, 2017

ebidel May 10, 2017

paulirish commented May 10, 2017

robdodson commented May 10, 2017

paulirish commented May 11, 2017

dylanb commented May 11, 2017

paulirish commented May 13, 2017

paulirish commented May 13, 2017

paulirish commented May 15, 2017

patrickhulce commented May 15, 2017

dylanb commented May 19, 2017

Add DomNode renderer #2206

Add DomNode renderer #2206

Conversation

paulirish commented May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 10, 2017

robdodson commented May 10, 2017

paulirish commented May 11, 2017

dylanb commented May 11, 2017

paulirish commented May 13, 2017

paulirish commented May 13, 2017

paulirish commented May 15, 2017

patrickhulce commented May 15, 2017

dylanb commented May 19, 2017

paulirish commented May 10, 2017 •

edited

Loading