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

Color fixes: #607, #556, #596 #610

Merged
merged 7 commits into from
Dec 12, 2017
Merged

Color fixes: #607, #556, #596 #610

merged 7 commits into from
Dec 12, 2017

Conversation

marcysutton
Copy link
Contributor

@marcysutton marcysutton commented Nov 8, 2017

I fixed two separate color contrast bugs:

  • The sibling of a disabled input inside of a label should be exempt
  • Inline elements spanning multiple lines shouldn't be incomplete

I decided against rewriting the color contrast algorithm entirely at this time, since the fix for #607 was as simple as adding a target to the background stack. No need to walk the DOM if I'm already on the element that should have been grabbed by document.elementsFromPoint. Some day we might want to rewrite the algorithm to walk and test for relevant colors in one step rather than gathering and checking colors after, but that was overkill for these changes.

@@ -121,6 +121,9 @@ function elmPartiallyObscured(elm, bgElm, bgColor) {
* @param {Element} elm
*/
function includeMissingElements(elmStack, elm) {
if (!elmStack.includes(elm)) {
elmStack.unshift(elm);
Copy link
Contributor Author

@marcysutton marcysutton Nov 8, 2017

Choose a reason for hiding this comment

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

Can you foresee any problems with putting the target at the front of the element stack? Do we need to check for z-index? This is shimming a failure by document.elementsFromPoint to pick up inline elements spanning line breaks. We aren't checking the style of anything until later...

Copy link
Contributor

Choose a reason for hiding this comment

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

Since z-index could be manipulated by the page's styles the target element might actually be behind other elements in the stack. That seems problematic to me instinctually, but I am probably missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add another test for that case and see if it causes problems.

let rect = elm.getBoundingClientRect();
// allows inline elements spanning multiple lines to be evaluated
let multipleRects = elm.getClientRects();
let rect = multipleRects.length > 1 ? multipleRects[0] : elm.getBoundingClientRect();
Copy link
Contributor Author

@marcysutton marcysutton Nov 9, 2017

Choose a reason for hiding this comment

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

I found that if I used getClientRects() to gather an x, y to pass to document.elementsFromPoint, we could leverage that method to find elements on top. The only downside I could see is if a point in rects[1+] has a difference in element overlap, the background stack could be different for parts of a single node. A scenario could be if an inline element spanning multiple lines is only partially covered–we could potentially put it into incomplete if there's more than one clientRect, but we'd have to check if something with color overlaps partially for that result to be helpful. That seemed like overkill to me at this time.

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 there's a possible false positive we're ignoring if we're not checking each of those clientRects. I don't think we should put them all in incomplete. Instead I'd suggest we do elementsFromPoint on each of them, and only run it into incomplete if the resulting arrays don't match. WDYT?

Copy link
Contributor Author

@marcysutton marcysutton Nov 13, 2017

Choose a reason for hiding this comment

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

@WilcoFiers I found one wrinkle in this plan: if the inline element has a line break in it (a la <br>), getClientRects() includes an extra, but different, rect for it..at least in Chrome. Perhaps I need to exclude that case, since it doesn't add anything relevant in terms of text content impacted by color? It would be a shame for axe-core to get hung up on a line break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also need a method for comparing arrays of DOM objects, if you have any performant suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

No preference. Shallow compare is pretty easy though:

arr1.length == arr2.length && arr1.every((val, i) => val === arr2[i])

I wouldn't try to optimise this. This isn't nearly as costly as many other things the color-contrast rule is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, that's really similar to what I ended up doing. But I still have to do a lot of finagling with the data structure. I'm getting close though.

@marcysutton
Copy link
Contributor Author

marcysutton commented Nov 24, 2017

This PR is so close, but the trickiest of the tests are failing in PhantomJS and not in the browser (of course). This code is purely concerned with collecting the background stack, not about checking computed style. Here's the gist of the changes I had to make to incorporate Wilco's feedback:

  • If an inline link spans more than one line, it has multiple clientRects. Check the background stack returned from each DOMRect's call to document.elementsFromPoint. If the elements in the arrays match, use the first array.
  • If they don't match, pass a null result to trigger a review item and set elmPartiallyObscuring for remediation tips.

In writing the logic and tests for this, I found use two cases in conflict with each other:

  • multiline inline links that aren't fully covered by a background (these should be undefined/incomplete)
  • default implicit label wrapping an input (this should be true)

client rects outlined in element fragments

The default implicit label has multiple clientRects in Chrome/Safari, with the input being the only difference. In contrast, the multiline inline link gets a slightly different background stack for each link DOMRect (the elements each fragment covers).

I saw multiple possibilities for discerning between the two cases, but the easiest was to check whether the element under test is in the bounding client rect stack. In every real browser I checked, the multiline link wasn't in the stack returned from elm.getBoundingClientRect(), so it was a differentiator from the inline label. I added another condition in the array matching bit to say if the element under test is in the stack from getBoundingClientRect, pass it. We check the computed style of overlapping elements later.

But in Phantom this doesn't work, and debugging it is a huge PITA. I have to put this down again to get ready for dotJS, but this is as far as I got.

Here's a codepen showing the essential problem: https://codepen.io/marcysutton/pen/pdjNqK?editors=0100

Marcy Sutton added 3 commits November 24, 2017 15:59
Elements spanning multiple lines now pass coordinates from their first box/rectangle to document.elementsFromPoint for gathering an element stack.
assert.equal(Math.round(actual.green), 0);
});

it('should return null for a multiline block element not fully covering the background', function () {
Copy link
Contributor Author

@marcysutton marcysutton Dec 7, 2017

Choose a reason for hiding this comment

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

This test fails in Firefox AND Phantom, which is super irritating. Essentially, this test returns an actual color even though half of the element isn't covering its parent–but in Chrome it returns null. Rather than spending more time trying to fix this case, I'm going to capture the problem in #621 and remove the test from this PR.

TODO: address in #621
// default case, elm.getBoundingClientRect()
return rectStack[0];
}
else if (rectStack && rectStack.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor formatting issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@marcysutton marcysutton merged commit f3bd2f5 into develop Dec 12, 2017
@marcysutton marcysutton deleted the color-fixes branch December 15, 2017 22:21
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