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): account for elements that do not fill entire bounding size #3186

Merged
merged 2 commits into from
Oct 6, 2021

Conversation

straker
Copy link
Contributor

@straker straker commented Oct 5, 2021

Closes issue: #3166

@straker straker requested a review from a team as a code owner October 5, 2021 17:11
@@ -360,6 +360,8 @@ function addNodeToGrid(grid, vNode) {
const endRow = ((y + rect.height) / gridSize) | 0;
const endCol = ((x + rect.width) / gridSize) | 0;

grid._numCols = Math.max(grid._numCols ?? 0, endCol);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a little off. If _numCols needs to be different, it should be fixed where that value is set. If it doesn't need to be changed, this should probably just be a local variable.

Copy link
Contributor Author

@straker straker Oct 6, 2021

Choose a reason for hiding this comment

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

It needs to be set for every grid, so can't be local. Grid items are added dynamically, so there's no way to know how many rows or columns the grid will have before hand, we can only know as each item is added and changes the grid size.

lib/commons/dom/get-rect-stack.js Outdated Show resolved Hide resolved
// can't be an element whose midpoint is at column 5. If this happens this
// means there's an error in our grid logic that needs to be fixed
if (row > grid.cells.length || col > grid._numCols) {
throw new Error('Element midpoint exceeds the grid bounds');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow why you would want this to throw. Crashing axe-core doesn't seem like the right solution 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.

Because I want to know when our custom grid logic fails, and it shows there's an issue that needs to be fixed. Without throwing, color contrast would hide issues that should be fixed.

@straker straker dismissed WilcoFiers’s stale review October 6, 2021 14:31

Resolved private

@straker straker merged commit 699697b into develop Oct 6, 2021
@straker straker deleted the color-contrast-empty-cells branch October 6, 2021 16:15
straker added a commit that referenced this pull request Oct 18, 2021
…nding size (#3186)

* fix(color-contrast): account for elements that do not fill entire bounding size

* not private
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.

2 participants