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 contrast: text that overflows its parent should go into incomplete #621

Closed
marcysutton opened this issue Nov 22, 2017 · 2 comments
Closed
Labels
color contrast Color contrast issues

Comments

@marcysutton
Copy link
Contributor

marcysutton commented Nov 22, 2017

In working on #610, we found another use case that isn't quite covered: if a block element with text is positioned over an element with a background color but the text overflows, the current color algorithm causes a false positive for the overflowed text. It should return null instead of returning the background color, since we can't really tell. We talked about doing some overflow testing of the text node.

Here's a test that fails both in develop and in the color fixes PR. I didn't want to let it hold up the work that's been ongoing for a while:

it('should return null for a multiline block element not fully covering the background', function () {
	fixture.innerHTML = '<div style="position:relative;">' +
		'<div id="background" style="background-color:rgba(0,0,0,1); position:absolute; height:20px;"></div>' +
		'<p style="position: relative;z-index:1;" id="target">Text content Text content Text content Text content Text content Text content</p>' +
	'</div>';
	var actual = axe.commons.color.getBackgroundColor(document.getElementById('target'), []);
	assert.isNull(actual);
	assert.equal(axe.commons.color.incompleteData.get('bgColor'), 'elmPartiallyObscured');
});

The passing version in #610 works when the target has a defined width so the line breaks, and it goes through the new logic for multiple clientRects. But this one doesn't have multiple rects, so we have to look at the text node itself.

@marcysutton marcysutton added the color contrast Color contrast issues label Nov 22, 2017
marcysutton pushed a commit that referenced this issue Dec 7, 2017
TODO: address in #621
@marcysutton
Copy link
Contributor Author

When I looked into fixing this (but decided to wait), the most relevant section of code was the contentOverlapping function in get-background-color.js which only looks at the top left corner.

Perhaps we should sample the element stack from more than one corner, and put it into incomplete if the arrays don't match. We have to weigh this with performance, of course...but it's a similar problem to the inline element checking I had to do. One thing that makes this quite difficult is that different browsers have different results with getClientRects(). Firefox returns a single rect, while Chrome/Safari return multiple. That's primarily why I declined to move forward with fixing it now.

marcysutton pushed a commit that referenced this issue Dec 12, 2017
marcysutton pushed a commit that referenced this issue Dec 12, 2017
TODO: address in #621
marcysutton added a commit that referenced this issue Dec 14, 2017
* fix(color-contrast): incl. elements w/ line breaks

Closes #607
Closes #556

* fix(color-contrast): allow disabled label children

Closes #596

* fix: adjust color algorithm for inline elements

Elements spanning multiple lines now pass coordinates from their first box/rectangle to document.elementsFromPoint for gathering an element stack.

* fix: handle contrast of multiline inline el's

* test: ignore Phantom's LIES about color contrast

* test: remove failing test

TODO: address in #621

* chore: fix formatting issue
@WilcoFiers
Copy link
Contributor

Not sure how or when, but this seems to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color contrast Color contrast issues
Projects
None yet
Development

No branches or pull requests

2 participants