-
Notifications
You must be signed in to change notification settings - Fork 783
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
fix: corrects color-contrast tests with overlapping elements #378
Conversation
ee69497
to
c8f432b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests still pass, so the original fix for implicit labels seems to be preserved without needing getClientRects
. Nice job 👍
// get content box of target element | ||
// check to see if the current bgNode is overlapping | ||
var targetRect = targetElement.getClientRects()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I used getClientRects
because it would target the diff in coverage for an inline label wrapping an input, as opposed to the outer bounding box of the label. I guess with this change, the fgNode
is the input instead of the label in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fgNode
in this case would be the foreground "whatever". The function receives two element parameters - one that's the "background" and one that's potentially overlapping (generally a child element). The only way you'd have overlapping would be with absolute positioning, but you'd still need a parent/child combo, right? Are there any scenarios that might not be covered here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the input was under audit and its parent was an implicit, inline label, the label would be skipped and not considered a background unless it was added to the stack manually. (also the label itself as a node under audit, which contains the actual text content). I couldn't guarantee which element was in the front of the element stack since the node context would be either the child or the parent. If you look at the examples in this MDN doc, you can see getClientRects
can be used to check inline content overlap. https://developer.mozilla.org/en-US/docs/Web/API/Element/getClientRects
But if this works, awesome. I want to run it through the aXe extension to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this broke a few tests in Chrome (v59). We used to only get one error there (which I'm fixing in #373), but now I'm getting 5 failures. Please keep in mind that grunt test-webdriver
is currently broken, so you'll need to go over the results yourself to check. Firefox also has errors it didn't used to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see the test failures now–there are quite a few when you run grunt test
. If you rebase from develop, you can fix some of the flaky iframe tests. But there are some legit failures in both the unit and integration tests.
c8f432b
to
9a665e1
Compare
I believe this corrects the color-contrast check that tests for overlapping and obscuring elements. The previous code was checking for elements only at a specific set of coordinates, not within the bounding box. So, for example, if there was nothing at
0,0
then it would pass, even if there was overlap elsewhere.Supporting documentation: https://stackoverflow.com/a/12067046
One nit change: I updated
elm
tonode
to make it more clear what is coming in (the node being checked). Happy to revert this. No additional changes were done.