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

fix(color-contrast): correctly calculate background color of text nodes with different size than their container #3703

Merged
merged 40 commits into from
Dec 8, 2022

Conversation

straker
Copy link
Contributor

@straker straker commented Oct 7, 2022

Had an idea on how to improve the accuracy of our color contrast algorithm by looking at just the text rects (instead of trying to look at the bounding box of the element). This not only simplifies the algorithm a bit by removing a few in-between functions, but also fixes incorrect calculations of background color, such as in the following example which would report no contrast issues even though the text is fully on top of a dark background:

<div id="background-1" style="background-color: #222; height: 40px; width: 100px"></div>
<div id="foreground-div" target style="margin-top: -40px;">Hello world<br/>Goodbye Fate</div>

Also fixes a lot of different cases reporting partial overlap when they didn't need to.

This also deprecates visually-contains, filteredRectStack, and getRectStack, but I'll deprecate them in a separate pr.

Closes issue: #3626
Closes issue: #3442
Closes issue: #3400
Closes issue: #3257

@straker straker changed the title fix(color-contrast): correctly calculate background color of text nodes with different size than their container feat(color-contrast): correctly calculate background color of text nodes with different size than their container Oct 7, 2022
@straker straker changed the title feat(color-contrast): correctly calculate background color of text nodes with different size than their container fix(color-contrast): correctly calculate background color of text nodes with different size than their container Oct 21, 2022
@straker straker force-pushed the color-contrast-text-rects branch from 092ff52 to 9c79ddb Compare October 24, 2022 21:52
@straker straker marked this pull request as ready for review November 2, 2022 22:18
@straker straker requested a review from a team as a code owner November 2, 2022 22:18
lib/commons/math/get-intersection-rect.js Outdated Show resolved Hide resolved
lib/commons/math/get-intersection-rect.js Outdated Show resolved Hide resolved
lib/commons/math/get-intersection-rect.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-text-rects.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-text-rects.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-stack.js Show resolved Hide resolved
lib/commons/color/get-background-color.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-color.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-color.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-stack.js Outdated Show resolved Hide resolved
straker and others added 2 commits November 8, 2022 08:19
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
lib/commons/color/get-background-color.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-text-rects.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-text-rects.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-stack.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-color.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-text-rects.js Outdated Show resolved Hide resolved
straker and others added 6 commits November 29, 2022 16:22
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
lib/commons/color/get-background-color.js Show resolved Hide resolved
lib/commons/color/get-background-color.js Show resolved Hide resolved
lib/commons/color/get-background-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-text-element-stack.js Outdated Show resolved Hide resolved
lib/commons/color/get-background-color.js Show resolved Hide resolved
lib/commons/dom/get-visible-child-text-rects.js Outdated Show resolved Hide resolved
lib/commons/dom/get-visible-child-text-rects.js Outdated Show resolved Hide resolved
lib/commons/dom/get-text-element-stack.js Outdated Show resolved Hide resolved

const overflow = vNode.getComputedStylePropertyValue('overflow');

if (overflow === 'hidden') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a ticket for overflow-x: hidden overflow-y: visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WilcoFiers
WilcoFiers previously approved these changes Dec 8, 2022
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