Text spacing not !important [78fd32, 24afc2, 9e45ec]: Consider actual font size and ignore empty text #1804
Conversation
Jym77
added
Rule Update
| - <dfn id="24afc2:wide-enough-cond">wide enough</dfn>: the parent of the text node has [wide enough letter spacing](#24afc2:wide-enough-def); or | ||
| - <dfn id="24afc2:cascade">cascade</dfn>: the parent of the text node has a [cascaded][] [letter-spacing][] property which is not the one [declared][] in the `style` attribute of the target. | ||
|
|
||
| An element has a <dfn id="24afc2:wide-enough-def">wide enough letter spacing</dfn> if the computed value of its [letter-spacing][] property is at least 0.12 times the computed value of its [font-size][] property. |
There was a problem hiding this comment.
To mirror what we did with the line height we might want to make this the used value instead of the computed value.
There was a problem hiding this comment.
Fair point. It is not really needed for letter-spacing and word-spacing since the used value and computed value should be equal. But alignment is a good idea…
| - <dfn id="24afc2:spaced-text">spaced text</dfn>: for each text node descendant of this target, one of the following is true: | ||
| - <dfn id="24afc2:empty">empty</dfn>: the text node is only [whitespace][]; or | ||
| - <dfn id="24afc2:wide-enough-cond">wide enough</dfn>: the parent of the text node has [wide enough letter spacing](#24afc2:wide-enough-def); or | ||
| - <dfn id="24afc2:cascade">cascade</dfn>: the parent of the text node has a [cascaded][] [letter-spacing][] property which is not the one [declared][] in the `style` attribute of the target. |
There was a problem hiding this comment.
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.
Yes, the problem is that a single target can have all three cases (or more), something like:
<div style="letter-spacing: 2.4 px !important; font-size: 30px">
<span> </span> <!-- OK because too small but only whitespace -->
<span style="font-size: 20px">Lorem</span> <!-- OK because font-size has changed -->
<span style="letter-spacing: 3.6px !important">ipsum</span> <!-- OK because letter-spacing has changed -->
<span>dolor sit amet</span> <!-- bad -->
</div>The 4th case totally only fails because of the div, so we do want the div 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).
| if (stripRefNamesAndURLs) { | ||
| // Start of line, Opening [, no ] (repeated), closing ], /[[][^\]]*]: [^ ]*/ => matches "[name]: url" | ||
| // => the leftover bit is the title, which should be spellchecked. | ||
| // The name is only spellchecked if it appears in text, which is OK. | ||
| // 'm' flag checks in multiline mode, aka each line is matched separately. | ||
| const refListRegex = /^\[[^\]]*]: [^ ]*/gm | ||
| body = body.replace(refListRegex, "") | ||
| } |
There was a problem hiding this comment.
Adding code to ignore the names and URL parts of reference list items when spellchecking, i.e. in
`[name]: url 'title'
only the title needs to be spellchecked (the name being spellchecked if it appears in the text). This became useful here with all the #xxxxxx:anchor links in the reference list.
|
|
||
| #### Passed Example 13 | ||
|
|
||
| This `p` element has three text node descendants. The first one is [empty](#24afc2:empty); the second one is [wide enough][] because of the smaller `font-size` on its parent; and the last one is [wide enough][] because of the `letter-spacing` specified on its parent. The third `span` element has a [wide enough][] text node descendant. |
There was a problem hiding this comment.
I feel like it would be good to specifically highlight there are two test targets in this example. Also, added the reference to the cascade condition.
| This `p` element has three text node descendants. The first one is [empty](#24afc2:empty); the second one is [wide enough][] because of the smaller `font-size` on its parent; and the last one is [wide enough][] because of the `letter-spacing` specified on its parent. The third `span` element has a [wide enough][] text node descendant. | |
| This `p` element has three text node descendants. The first one is [empty](#24afc2:empty); the second one is [wide enough][] because of the smaller `font-size` on its parent; and the last one is [wide enough][] because of the `letter-spacing` specified on its parent, meeting the [cascade](#24afc2:cascade) condition. The third `span` element, which is also applicable, has a [wide enough][] text node descendant. |
There was a problem hiding this comment.
For the p target, the text inside the third span is actually meeting both the "cascade" condition and the "wide enough" one (because the parent of this text node, the span, has a used letter-spacing of 3.6px, and a computed font-size of 30px; and 30×1.2 ⩽ 3.6).
I felt that "wide enough" is easier to understand 😅 But I guess we can keep both…
|
|
||
| #### Passed Example 13 | ||
|
|
||
| Both this `p` and `span` elements have a single text node descendant, whose parent (the `span` element) is [large enough][]. |
There was a problem hiding this comment.
Shouldn't the description here mention that the p passes because of the cascade condition? The large enough condition is only relevant for the span
There was a problem hiding this comment.
Here also, the large enough condition is relevant for both 🤷
There was a problem hiding this comment.
This is getting really very complicated. I'm not sure this is actually necessary. To be honest I'd much rather we do this by just adding an assumption. Previously the rule of thumb has been that if we have no real-world examples of a problem, we're can use those. WDYT?
I'm not sure how much of an actual problem the full rule detects in the first place 😅 Thus said, I feel the way to simplicity would be to move focus of the rule from the element to the text node (so from where the problem is created to where it actually occurs): Applicability: any (parent of a) (non-whitespace) text node. Expectation: either (i) specified This immediately rules out the corner cases where a definition is not affecting any text, and freely handle the cases where the properties get changed. Maybe that would need to deprecate these rules and create new ones since it is a significant change 🤔 @WilcoFiers what do you think about this? |
Jym77
dismissed
carlosapaduarte’s stale review
Opinion wanted on latest discussion.
|
@Jym77 Just for how much that changes these rules, I'd rather not. If we wanted to fix this (and I'm not convinced we need to) perhaps the way to do it would be by doing this: - **wide enough**: the [computed][] [letter-spacing][] is at least 0.12 times the [computed][] [font-size][] of all descending text node in the flat tree; or |
That fixes #1688 ( |
|
How about wording like this:
The avoids having to embed details of how the CSS the cascade works in the rule, and lets implementors choose how they implement this (looking at cascaded value, looking at specificity, calling an API, etc) The definition above seems to work for all these cases: |
I'm not sure what "the computed value was assigned from" means. I guess i will boil down to something like "the specified value was declared in" (an |
|
assign comes from here:
https://www.w3.org/TR/css-cascade-3/#value-stages Here's a tweaked version with a link to the definition:
|
|
Closed by #1923 |
Superceded by #1923
Currently updating only "Line height is not
!important" to check whether that solution works or not.The second and third condition where about the test target, which is incorrect since both the
font-sizeor the text spacing property can be updated before the actual text. This replaces them with a condition that look at these properties on the parent of each text node inside the test target.Closes issue(s):
Need for Call for Review:
This will require a 2 weeks Call for Review (big change to expectation reworking the logic significantly)
Pull Request Etiquette
When creating PR:
developbranch (left side).After creating PR:
Rule,DefinitionorChore.When merging a PR:
How to Review And Approve