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

feat: Add shadow DOM to duplicate-img-label check #443

Merged
merged 2 commits into from
Jul 18, 2017

Conversation

WilcoFiers
Copy link
Contributor

Should close #430.

@WilcoFiers WilcoFiers requested a review from dylanb July 17, 2017 12:38
// Get all visible images in the composed tree of the current node
const images = axe.utils.querySelectorAll(virtualNode, 'img')
// Ignore hidden or role=none/presentation images
.filter(({ actualNode }) => (axe.commons.dom.isVisible(actualNode) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dylanb Not sure if this was a bug, or if this is intentional. isVisible has a screenreader flag, should this function use that flag? I'm not sure why we care here if the image is positioned off screen or not. Thoughts?

Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

forcing to boolean (plus undefined) would be better than the truthy return value but not required


return false;
// See if any of the images duplicate the node's text
return images.some(img =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

array.some returns a boolean.

var node = axe.utils.querySelectorAll(axe._tree[0], target || '#target')[0];

var node;
if (typeof target === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We needed this testutils file after all 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is very nice to have hanging around.

@WilcoFiers WilcoFiers merged commit 2c0b075 into shadowDOM Jul 18, 2017
WilcoFiers added a commit that referenced this pull request Jul 18, 2017
* feat: Add shadow DOM to duplicate-img-label check

* fix: Use tabs
@dylanb dylanb deleted the sd/duplicate-img-label branch July 18, 2017 12:42
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