Limit contrast checker to only blocks that have at least one color defined#43592
Merged
Conversation
|
Size Change: +18 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Contributor
|
Thanks for taking a stab at this, and thank you for the video before/after. Just based on those, the "after" seems vastly preferable simply because the contrast warning shows up in context of the offending colors, and nowhere else.
Can you expand a little bit on this? |
Contributor
Author
The List block is a good example of this. Without this change, the link contrast color checking works how you would expect it to, eg. works within the context of the current block: link-color-before.mp4but with this change, because the link-color-after.mp4the trick is in finding a way to scope the contrast checker to just the current block structure, and ignore nested blocks, which I can't see an easy way of doing currently, so it may be a case of picking the lesser of the two evils in the interim - or somebody else may have some ideas about how to better handle this. |
Contributor
|
That's a good example illustrating how gnarly this calculation is! In case it helps, I perceive the biggest challenge of #43537 to be about the UI. The case where you see unset colors with a warning should never occur, because in that context, none of those colors can be at fault:
Would a solution be to add a case to the notice to just be hidden if the above colors are unset? |
Contributor
Author
Yeh, that would be worth exploring. I am AFK most of next week, but will take a look at that the week after if someone else doesn't get to it before that. |
Contributor
Author
|
@jasmussen I have updated this so the contrast checker doesn't run if a block has not defined any colors, it only kicks in if at least one color is defined. It is better, as at least no error shows when nothing defined, but still a little confusing with nested checking, eg. if you define a light background on a group, when a nested paragraph with dark background has light links: contrast.mp4I didn't think we are going to find a perfect solution for this, but not contrast checking until a user actually selects a color on a block as the PR now does may be a worthwhile first step? |
eb40c1d to
459e22a
Compare
jasmussen
approved these changes
Sep 7, 2022
jasmussen
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the latest update, and the video. I can confirm that the contrast checker only appears in context of the problematic color:
but still a little confusing with nested checking, eg. if you define a light background on a group, when a nested paragraph with dark background has light links:
That's the use case I tested above, and I genuinely think the new behavior is clearer than before. Because if we show the contrast notice in context of a block with no colors defined, then that might be where you try to fix it — and there's no guarantee that would work at all: a black background group with white text would still not have sufficient contrast if the paragraph inside was set to dark blue.
In other words, in order for the information to be actionable to a user, it has to be shown in context of the problem source, which this gets us much closer to. So I think we should land this.
Small note from the GIF above, towards the end you'll see that the contrast notice does not clear immediately when I remove the background color, it only disappears when I deselect and select the block again. Approving this regardless, as it doesn't seem possible for that issue to stem from this PR.
One final note: we can do more in the future to improve actionability on contrast issues. One idea is to flag issues like these with a dot next to the offending block in the list view, or to surface a panel of "Issues to look at" in the pre-publish panel. We can even show a dot indicator on the List view itself to bring attention to a block that has an issue. So there are more ways to bring attention to contrast issues than the inspector notice.
459e22a to
1df2592
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What?
Limits the scope of
linkcolor check in the contrast checker to direct children of current blockWhy?
Currently it gets the first link found at any depth of nesting within a block and compares it against the computed background color of current block, ignoring all other backgrounds that may be set between the two, this results in both false positives and false negatives as noted here.
Fixes: #43537
How?
Adds a direct child scope to the link query selector
':scope > a'N.B. This isn't a perfect solution as it means the link contrast color will not be checked in cases where the link
atag is not a direct child of the parent block when colors are not explicitly set. At the moment it may be a case of deciding which is the least confusing as I can't think of a perfect fix.Testing Instructions
Screenshots or screencast
Before:
contrast-before2.mp4
After:
contrast-after.mp4