Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

False Negatives running in chrome browser, but not running through phantomjs #126

Closed
tute opened this issue Dec 23, 2014 · 8 comments · Fixed by #128
Closed

False Negatives running in chrome browser, but not running through phantomjs #126

tute opened this issue Dec 23, 2014 · 8 comments · Fixed by #128
Assignees

Comments

@tute
Copy link

tute commented Dec 23, 2014

When we do axs.Audit.run() to Google.com as in #125 we get a few FAILs, but if we run the same script in google.com in Chrome's console we do not:

screen shot 2014-12-23 at 4 56 14 pm

This may be relevant and high priority for the Chrome Extension.

@ckundo
Copy link
Collaborator

ckundo commented Dec 23, 2014

@alice the Chrome Extension seems to be directly impacted by this bug.
screen shot 2014-12-23 at 5 01 29 pm

@ckundo
Copy link
Collaborator

ckundo commented Jan 2, 2015

@alice the Chrome extension is working properly after all on my machine - Adblock was interfering with ADT, everything worked after disabling adblock. @tute are you using Adblock?

@ricksbrown
Copy link
Collaborator

The problem appears to be in axs.AuditRule.collectMatchingElements in particular in the way it handles shadow DOM.

I think this assumption is wrong:

// Descend into node:
// If it has a ShadowRoot, ignore all child elements - these will be picked
// up by the <content> or <shadow> elements. Descend straight into the
// ShadowRoot.

An element could have both regular DOM child elements AND shadow DOM.
When Adblock is installed the documentElement (the HTML element) has a shadowRoot - the entire DOM is therefore not audited, only the shadow DOM.

It could be as simple as commenting out the return statement on line 148 of AuditRule.js.

@ricksbrown ricksbrown self-assigned this Jan 5, 2015
ricksbrown added a commit to ricksbrown/accessibility-developer-tools that referenced this issue Jan 5, 2015
@ricksbrown
Copy link
Collaborator

@ckundo I fixed this in my existing pull request - sorry I wasn't sure how to create an entirely isolated pull request from my fork.

I added some unit tests but they only function when run on a platform that supports shadow dom - in other words they do nothing in the current version of phantomjs.

ricksbrown@3a3da9a

@alice
Copy link
Contributor

alice commented Jan 6, 2015

@ricksbrown Could you pull this fix into a separate branch and create a new pull request for it? (Create a new git branch locally, push that up to your repo, and you should see an option in the UI to create a new PR.)

@alice
Copy link
Contributor

alice commented Jan 6, 2015

@ricksbrown Also, I checked, and it's not quite accurate that "An element could have both regular DOM child elements AND shadow DOM" - what's happening with AdBlock Plus is that it's inserting a shadow root containing only a <shadow> element, and the <shadow> element is hosting the rest of the content. (You can check this out by deleting the <shadow> element using devtools - the page disappears.)

So what actually needs to be fixed is that in this if-block https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/AuditRule.js#L169 we need to check first for olderShadowRoot, but then check for distributed nodes as per https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/AuditRule.js#L157

(I couldn't understand the spec, so I asked one of the spec authors to explain it to me... perks of working where I do!)

ricksbrown added a commit to ricksbrown/accessibility-developer-tools that referenced this issue Jan 6, 2015
@ricksbrown
Copy link
Collaborator

@alice I created a pull request but hadn't read your additional comments. I'll rework the pull request, ignore it for the moment, I'll post back here when I'm done.

ricksbrown added a commit to ricksbrown/accessibility-developer-tools that referenced this issue Jan 6, 2015
@alice
Copy link
Contributor

alice commented Jan 6, 2015

@ricksbrown I've also been fiddling with this (based off your original PR) - depending how things go I might send you a PR to incorporate into your PR :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants