Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

fix: Fix false positive by ignoring text that lives over an image overlay #60

Closed
wants to merge 5 commits into from

Conversation

DaveTryon
Copy link

@DaveTryon DaveTryon commented Jan 27, 2020

Closes: #59

Note: This adds a threshold value of 100,000, which is somewhat arbitrary based on the images in the unit tests. The largest value reached in the current unit tests is 61.425. There's a possibility of a non-overlay text reaching this threshold--if that occurs, then that scanline will be ignored and scanning will continue with other scanlines. It's a heuristic, but intended to err on the side of fewer false positives.

Requester Review Items

  • Ensure the Pull Request is into develop.
  • Test coverage has not gone down.
  • Documentation updated or not needed. (Wiki, Issues, etc)
  • Angular Type leads to proper Semantic Version.

Dev Team Review Items

To be filled out by @dequelabs/mobile.

  • Ensure the Requester did not lie. Chastise them politely if they did.
  • Code Quality review.
  • Changes to Axe*.java classes are minimal, appropriate, and versioned properly.
    • Serialized Axe objects have not changed.
    • Public API has not changed.

Code Owner Review Items

To be filled out by @dequelabs/mobileadmin.

  • Any Rules package changes lead to proper Semantic Version.
  • Security Review.

Merger Review Items

  • Ensure commit message matches title before squashing.

@DaveTryon DaveTryon requested a review from a team as a code owner January 27, 2020 21:06
@DaveTryon DaveTryon changed the title Fix false positive by ignoring text that lives over an image overlay fix: Fix false positive by ignoring text that lives over an image overlay Jan 28, 2020
@chriscm2006 chriscm2006 self-assigned this Jan 30, 2020
@chriscm2006 chriscm2006 added the Fix Something isn't working label Jan 30, 2020
@chriscm2006
Copy link
Contributor

chriscm2006 commented Jan 30, 2020

Thanks @DaveTryon I'm going to take a look at this this week.

@chriscm2006 chriscm2006 reopened this Jan 30, 2020
@chriscm2006
Copy link
Contributor

@DaveTryon I don't think this is the way we should solve this problem, though your test case is super helpful! Would you mind if I pulled your branch down and tweaked some other paramaters and asked you to review them?

The problem your test case exhibits is solved not by "overlay detection" but by our gradient background logic which "should" but obviously isn't handling this scenario. I would prefer to tweak existing parameters to creating a new one. The problem with the logic as you've tweaked it, is that it will filter out many cases that we are currently detecting correctly.

If this is good with you, I'll take your logic and ensure your test case works, but by tweaking some other parameters. If there are any other test cases you want to add before I do this let me know!

@DaveTryon
Copy link
Author

@chriscm2006, I agree that this is not an elegant fix. If there's a way to adjust the gradient algorithm to handle this case, then I'm all for that. Please feel free to fork this branch--I'll add a few more images to serve as test cases, just to broaden the data set that gets checked. I'll leave the PR open until I get the chance to add the additional test cases (probably on 1/31)

@DaveTryon
Copy link
Author

@chriscm2006, I've added a few more test cases to https://github.com/DaveTryon/axe-android/tree/TextOverlayBugDemo. Please feel free to use these as appropriate to tune the color contrast handling. I noticed that some of the new test cases report low confidence instead of high, so the final form of the test cases may need to be tweaked accordingly. My intent here was to capture some interesting test cases. I'll close out this PR now.

@DaveTryon DaveTryon closed this Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColorContrast - Image underlays should be filtered out
2 participants