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.
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
Text spacing not !important [78fd32, 24afc2, 9e45ec]: Consider actual font size and ignore empty text #1804
Text spacing not !important [78fd32, 24afc2, 9e45ec]: Consider actual font size and ignore empty text #1804
Changes from 3 commits
fe1a070
a5baf5b
1f2de6f
b054813
0465c04
6560d84
8a4f431
f3c31b0
210204e
16d98b2
583cca6
88167d5
d6124c8
28add2e
cff3995
2c22816
4b2ecea
b4f9fe8
94a510c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been wrestling with this definition a bit. I finally understand it, but it seems weird since it feels like it is almost defining an inapplicable case and then calling it a pass (e.g., the text node doesn't use the target elements style which results in a pass). I'm not sure I have a suggestion for this, but figured I'd leave my thoughts in case it helps to figure out how to clarify this language somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the problem is that a single target can have all three cases (or more), something like:
The 4th case totally only fails because of the
div
, so we do want thediv
as a target.We could update applicability to say something like "…and has at least one text node descendant which is not only whitespace and whose parent's cascaded
letter-spacing
is not the one declared here". That would make cases with only the first or third case Inapplicable, which is quite good (these are cases where the!important
declaration does not affect anything. But we would still need to keep the "empty" and "cascade" exceptions in the Expectation due to the possibility of mixing things up, as here. Hence, I feel that doing so would result in complicating the Applicability without simplifying the Expectation, and duplicating some condition (making maintenance harder).Another solution could be to target the text nodes (which are not empty and whose parent's cascaded
letter-spacing
is!important
and declared in a style attribute), but that would result in failing the text nodes rather than the (element with the) bad declaration, so it is not that good for reporting. This would result in only targeting "Lorem" (pass) and "dolor sit amet" (fail).