fix(color-contrast): Prevent crash on large inline elments #1306 #1341
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WilcoFiers
commented
Jan 30, 2019
| } | ||
| }); | ||
|
|
||
| if (filteredArr.some(stack => stack === undefined)) { |
There was a problem hiding this comment.
Mostly a return-early refactor, these 3 lines are the new bit.
There was a problem hiding this comment.
Because the missing stacks could have issues on them, and ignoring those would mean we'd be ignoring potential color-contrast issues.
WilcoFiers
commented
Jan 30, 2019
| @@ -351,7 +352,8 @@ color.getBackgroundColor = function(elm, bgElms = [], noScroll = false) { | |||
| if (noScroll !== true) { | |||
| // Avoid scrolling overflow:hidden containers, by only aligning to top | |||
| // when not doing so would move the center point above the viewport top. | |||
| const alignToTop = elm.clientHeight - 2 >= window.innerHeight * 2; | |||
| const clientHeight = elm.getBoundingClientRect().height; | |||
| const alignToTop = clientHeight - 2 >= window.innerHeight * 2; | |||
There was a problem hiding this comment.
clientHeight is 0 on inline elements with multiple rects. getBoundingClientRect does give the rect for the entire thing.
stephenmathieson
approved these changes
Jan 30, 2019
| } else { | ||
| return [boundingStack]; | ||
| } | ||
| if (!boundingCoords) { |
There was a problem hiding this comment.
👍 thank you for this. Indenting the happy path always bugs me.
dylanb
reviewed
Feb 1, 2019
| let filteredArr = rects | ||
| .filter(rect => { | ||
| // exclude manual line breaks in Chrome/Safari | ||
| return rect.width && rect.width > 0; |
There was a problem hiding this comment.
nit: Why not just return rect.width > 0?
There was a problem hiding this comment.
We could. I just changed indentation on this. Didn't actually look to refactor.
dylanb
approved these changes
Feb 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Worked out the problem behind this issue that Stephen created a patch for 2 weeks ago. Basically, inline elements can have multiple rects when they wrap over multiple lines. This can happen in 2 cases. Either the inline element has rects that don't fit the viewport at all (which is very unlikely), or axe-core doesn't scroll the entire element into view (which was the case for the reported issue).
The reason why axe-core doesn't always scroll the entire element into view is because we don't use
.scrollIntoView(true)(alignToTop) most of the time. This is to avoid scrolling elements with overflow: hidden. There is probably a way to avoid this, and scroll the entire element into view, but that would require a lot more work. Probably not worth the effort, given how uncommon this issue is.Closes #1306, closes #1259
Reviewer checks
Required fields, to be filled out by PR reviewer(s)