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

Incomplete color-contrast check #1730

Closed
muan opened this issue Jul 29, 2019 · 10 comments
Closed

Incomplete color-contrast check #1730

muan opened this issue Jul 29, 2019 · 10 comments
Assignees
Labels
color contrast Color contrast issues docs Documentation changes fix Bug fixes rules Issue or false result from an axe-core rule support
Milestone

Comments

@muan
Copy link

muan commented Jul 29, 2019

Reference: #1539 and #1185.

We are observing a flaky color-contrast check that gets triggered due to viewport size changes. Initially we were using v3.1.2, and got the error in #1185.

I have now created a reproduction case with v3.3.1, see https://html-is.glitch.me/axe-core-color-contrast.html. In Chrome, these are the results I am getting:

When viewport width is 800px, 1 incomplete check is reported.

When viewport width is 850px, 1 violation and 1 incomplete color-contrast check are reported.

When viewport width is < 760px or > 900px, 1 color-contrast violation is reported.

The expected result is there should always be 1 color-contrast violation reported on this page.

I tried to debug this, and it comes down to the scrollIntoView call.

When axe-core calls scrollIntoView on the sidebar <span> element at 800px & 850px, nothing happens, because the element is partially visible already. This breaks the check because the element contains a child element which is outside the viewport. Changing the scrollIntoView call to (https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView):

elm.scrollIntoView({block: alignToTop ? 'start' : 'end', inline: 'end'});

fixes the issue, as it will ensure that the element is always scrolled fully into view if possible.

I'm not sure what browser compatibility is required of this check for axe-core, and in Safari I actually consistently get Element's background color could not be determined because it partially overlaps other elements regardless of viewport size. So the fix would definitely require a deeper look.

@straker straker added the info needed More information or research is needed to continue label Jul 30, 2019
@straker
Copy link
Contributor

straker commented Jul 30, 2019

Thanks for helping us better find where color-contrast is being a bit flaky. We'll have to figure out what exactly is going on, and now that we have a reproducible test case it will help us a lot.

@straker straker added fix Bug fixes rules Issue or false result from an axe-core rule labels Jul 30, 2019
@straker
Copy link
Contributor

straker commented Aug 3, 2019

So it looks like there's two different problems. The first is as you identified in that we need to scroll the element fully into view before we check for it (including horizontal scrolling). The second seems to be that Safari does not add the span parent tag of the button from the elementsFromPoint API, thus making it seem like there's a span overlapping the button in our checks.

@straker straker added color contrast Color contrast issues and removed info needed More information or research is needed to continue labels Aug 8, 2019
@haonliu
Copy link

haonliu commented Sep 27, 2019

we've experienced the same issue and tried the fix suggested by @muan and it works!

@jeankaplansky
Copy link
Contributor

jeankaplansky commented Oct 14, 2019

Include the title of this issue associated with the title/link to https://app.zenhub.com/workspace/o/dequelabs/axe-core/issues/1845 in release notes.

@chandana7393
Copy link

Tested, working as expected.
800px_width
600px_width
950px_width
850px_width

CC: @somaalapati
Tested Environment:
Axe-coconut - 4.0.0.29004v
Chrome - 77.0.3865.120v
OS - Windows 10 64 bit.

dbjorge added a commit to microsoft/axe-pipelines-samples that referenced this issue Oct 17, 2019
#### Description of changes

Per suggestion from @manishsat , this PR has the samples set a default window size explicitly, to avoid issues related to the color contrast test's "scroll into viewport" behavior in the default window sizes used by the browsers' headless modes (eg, dequelabs/axe-core#1730)

I verified the syntax of the window size parameters locally by temporarily disabling the headless options and manually verifying the created window sizes.

#### Pull request checklist

- [n/a] If this PR addresses an existing issue, it is linked: Fixes #0000
- [x] New sample content is commented at a similar verbosity as existing content
- [x] All updated/modified sample code builds and runs in at least one PR/CI build
- [x] PR checks for builds named `[failing example] ...` fail as expected
@devaradhanm
Copy link
Contributor

This issue is not yet fixed in 3.4.0. We have added a sample test that you can use to validate this here - devaradhanm/axe-puppeteer@ac3d4d5
We also validated that this test passes once we add the fix suggested by @muan
elm.scrollIntoView({block: alignToTop ? 'start' : 'end', inline: 'end'});

@WilcoFiers WilcoFiers reopened this Dec 2, 2019
@WilcoFiers WilcoFiers modified the milestones: Axe-core 3.4, Axe-core 3.5 Dec 2, 2019
@straker
Copy link
Contributor

straker commented Dec 2, 2019

@devaradhanm Can you provide a codepen that shows the problem? Stackoverflow shows 49 color contrast bugs for me but it's a responsive site that doesn't seem to have horizontal overflow at different sizes.

Testing the OPs pen using the latest axe extension at different screen sizes always shows 1 contrast violation. We can't solve the Safari bug in 3.4.0, but there are some changes being made to color-contrast for 3.5.0 that should solve that one.

Do note that elm.scrollIntoView({block: alignToTop ? 'start' : 'end', inline: 'end'}); is not supported in IE 11, so we had to instead write a workaround for the pr for this to work

@devaradhanm
Copy link
Contributor

@straker , by manual scanning via browser directly, I wasn't able to repro it. I was able to repro this issue when using puppeteer. You could get the changes I made on axe-puppeteer fork repo & just run the test - devaradhanm/axe-puppeteer@ac3d4d5

@ferBonnin
Copy link

@straker did the repo provided in the fork worked for you?

@padmavemulapati
Copy link

padmavemulapati commented Feb 5, 2020

I am not seeing any issue, tried with different viewport widths.. seeing only one voilation which is "Ensures the contrast between foreground and background colors meets WCAG 2 AA contrast ratio thresholds" .. @straker could you please confirm me anything else need to verify.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color contrast Color contrast issues docs Documentation changes fix Bug fixes rules Issue or false result from an axe-core rule support
Projects
None yet
Development

No branches or pull requests

10 participants