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.

Sign up for GitHub

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

Closed
wants to merge 19 commits into from
Closed

Text spacing not !important [78fd32, 24afc2, 9e45ec]: Consider actual font size and ignore empty text #1804

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fe1a070
Consider actual font size and ignore empty text
Jym77 Mar 3, 2022
a5baf5b
Merge branch 'develop' into style-not-important-fixes
Jym77 Mar 31, 2022
1f2de6f
Replace computed value by used value
Jym77 Mar 31, 2022
b054813
Merge branch 'develop' into style-not-important-fixes
Jym77 Apr 7, 2022
0465c04
Merge branch 'develop' into style-not-important-fixes
Jym77 Jun 9, 2022
6560d84
Merge branch 'develop' into style-not-important-fixes
Jym77 Jun 23, 2022
8a4f431
Add example passing thanks to all three spacing conditions
Jym77 Jun 23, 2022
f3c31b0
Ignore names and URLs in references list during spellcheck
Jym77 Jul 1, 2022
210204e
Use reference list for internal refs
Jym77 Jul 1, 2022
16d98b2
Clean up
Jym77 Jul 1, 2022
583cca6
Clean up
Jym77 Jul 1, 2022
88167d5
Fix properties changing values
Jym77 Jul 7, 2022
d6124c8
Using paragraphs in examples
Jym77 Jul 7, 2022
28add2e
Fix line-height or font-size changing
Jym77 Jul 7, 2022
cff3995
Merge branch 'develop' into style-not-important-fixes
Jym77 Jul 7, 2022
2c22816
Merge branch 'develop' into style-not-important-fixes
Jym77 Jul 14, 2022
4b2ecea
Merge branch 'develop' into style-not-important-fixes
Jym77 Jul 28, 2022
b4f9fe8
Merge branch 'develop' into style-not-important-fixes
Jym77 Aug 4, 2022
94a510c
Improve example descriptions
Jym77 Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions __tests__/spelling-ignore.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@
- 2px
- 3px
- 4px
- 10px
- 16px
- 20px
- 24px
Expand Down
14 changes: 13 additions & 1 deletion __tests__/spelling.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,28 @@ const checkSpelling = text => {
* @param {String} body body of markdown
* @param {Object} options options
* @property {Boolean} options.stripCodeBlocks boolean denoting if code blocks should be removed from content
* @property {Boolean} options.stripRefNames boolean denoting if names and URLs in the references list should be removed
* @returns {String}
*/
function getCuratedMarkdownBody(body, options = {}) {
const { stripCodeBlocks = true } = options
// A better approach would be to look into the markdown AST rather than the full body, and only spellcheck blocks
// of the correct types (paragraphs, headings, titles of links, …)
const { stripCodeBlocks = true, stripRefNamesAndURLs = true } = options

if (stripCodeBlocks) {
const codeBlocks = gfmCodeBlocks(body)
body = codeBlocks.reduce((out, { block }) => out.replace(block, ''), body)
}

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, "")
}
Comment on lines +107 to +114
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


return removeMd(body)
}

Expand Down
93 changes: 81 additions & 12 deletions _rules/letter-spacing-not-important-24afc2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@ This rule applies to any [HTML element][] that is [visible][] and for which the

For each test target, at least one of the following is true:

- **not important**: the [computed][] value of its [letter-spacing][] property is not [important][]; or
- **wide enough**: the [computed][] value of its [letter-spacing][] property is at least 0.12 times the [computed][] value of its [font-size][] property; or
- **cascade**: the [cascaded][] value of its [letter-spacing][] property is not a value [declared][] in its `style` attribute.
- <dfn id="24afc2:not-important">not important</dfn>: the [computed][] value of its [letter-spacing][] property is not [important][]; or
- <dfn id="24afc2:spaced-text">spaced text</dfn>: for each text node descendant of this target, one of the following is true:
Jym77 marked this conversation as resolved.
Show resolved Hide resolved
- <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.
Copy link
Collaborator

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.

Copy link
Collaborator Author

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:

<div style="letter-spacing: 2.4 px !important; font-size: 30px">
  <span>&nbsp;</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).


An element has a <dfn id="24afc2:wide-enough-def">wide enough letter spacing</dfn> if the [used][] value of its [letter-spacing][] property is at least 0.12 times the [computed][] value of its [font-size][] property.

## Assumptions

Expand Down Expand Up @@ -63,7 +67,7 @@ CSS specifications define each declaration as being either [important][] (if it

#### Passed Example 1

This `p` element has a **not [important][]** [computed][] `letter-spacing`.
This `p` element has a [not important][] [computed][] `letter-spacing`.

```html
<p style="letter-spacing: 0.1em">
Expand All @@ -73,7 +77,7 @@ This `p` element has a **not [important][]** [computed][] `letter-spacing`.

#### Passed Example 2

This `p` element has a [computed][] `letter-spacing` of 0.15 time the font size, which is **wide enough**.
This `p` element has a [computed][] `letter-spacing` of 0.15 time the font size, which is [wide enough][].

```html
<p style="letter-spacing: 0.15em !important">
Expand All @@ -83,7 +87,7 @@ This `p` element has a [computed][] `letter-spacing` of 0.15 time the font size,

#### Passed Example 3

This `p` element has a [computed][] [letter-spacing][] of `3px`, which is **wide enough** (the threshold is `3px`).
This `p` element has a [computed][] [letter-spacing][] of `3px`, which is [wide enough][] (the threshold is `3px`).

```html
<style>
Expand All @@ -99,7 +103,7 @@ This `p` element has a [computed][] [letter-spacing][] of `3px`, which is **wide

#### Passed Example 4

This `p` element has two [declared][] values for its `letter-spacing` property. The latest wins the [cascade sort][]. It has a value of `0.15em`, and is **wide enough**.
This `p` element has two [declared][] values for its `letter-spacing` property. The latest wins the [cascade sort][]. It has a value of `0.15em`, and is [wide enough][].

```html
<p style="letter-spacing: 0.1em !important; letter-spacing: 0.15em !important">
Expand All @@ -109,7 +113,7 @@ This `p` element has two [declared][] values for its `letter-spacing` property.

#### Passed Example 5

This `p` element has two [declared][] values for its `letter-spacing` property. The one which is [important][] wins the [cascade sort][]. It has a value of `0.15em`, and is **wide enough**.
This `p` element has two [declared][] values for its `letter-spacing` property. The one which is [important][] wins the [cascade sort][]. It has a value of `0.15em`, and is [wide enough][].

```html
<p style="letter-spacing: 0.15em !important; letter-spacing: 0.1em">
Expand All @@ -119,7 +123,7 @@ This `p` element has two [declared][] values for its `letter-spacing` property.

#### Passed Example 6

The [cascaded][] value of the `letter-spacing` property of this `p` element is [declared][] in the style sheet, not in the `style` attribute (it wins the [cascade sort][] because it is [important][]). Thus, the `p` element matches the **cascade** condition.
The [cascaded][] value of the `letter-spacing` property of this `p` element is [declared][] in the style sheet, not in the `style` attribute (it wins the [cascade sort][] because it is [important][]). Thus, the `p` element matches the [cascade](#24afc2:cascade) condition.

```html
<style>
Expand All @@ -135,7 +139,7 @@ The [cascaded][] value of the `letter-spacing` property of this `p` element is [

#### Passed Example 7

The [computed][] value of the `letter-spacing` property of this `p` element is **not [important][]**. The [computed][] value of the `letter-spacing` property of this `span` element is the [inherited][] value, that is the [computed][] value of its parent and therefore also **not [important][]**.
The [computed][] value of the `letter-spacing` property of this `p` element is [not important][]. The [computed][] value of the `letter-spacing` property of this `span` element is the [inherited][] value, that is the [computed][] value of its parent and therefore also [not important][].

```html
<p style="letter-spacing: 0.1em">
Expand All @@ -147,7 +151,7 @@ The [computed][] value of the `letter-spacing` property of this `p` element is *

#### Passed Example 8

The [computed][] value of the `letter-spacing` property of this `p` element is **not [important][]**. The [computed][] value of the `letter-spacing` property of this `span` element is the [inherited][] value, that is the [computed][] value of its parent and therefore also **not [important][]**.
The [computed][] value of the `letter-spacing` property of this `p` element is [not important][]. The [computed][] value of the `letter-spacing` property of this `span` element is the [inherited][] value, that is the [computed][] value of its parent and therefore also [not important][].

```html
<p style="letter-spacing: 0.1em">
Expand All @@ -157,6 +161,67 @@ The [computed][] value of the `letter-spacing` property of this `p` element is *
</p>
```

#### Passed Example 9

This `p` element only has one text node descendant, and it is [wide enough][] because its parent is the `span` element, with a `letter-spacing` of `2px`, more than `0.12` times the `font-size` of `10px`.

```html
<p style="font-size: 24px; letter-spacing: 2px !important">
<span style="font-size: 10px;">
The toy brought back fond memories of being lost in the rain forest.
</span>
</p>
```

#### Passed Example 10

This `p` element has no text node descendant and therefore matches the [spaced text](#24afc2:spaced-text) condition.

```html
<p style="letter-spacing: 1px !important; border-top: 1px solid black;">
<!-- empty div, border to make it "visible" -->
</p>
```

#### Passed Example 11

This `p` element only has one text node descendant, and it is [empty](#24afc2:empty).

```html
<p style="letter-spacing: 1px !important; border-top: 1px solid black;">
&nbsp:<!-- empty div, border to make it "visible" -->
</p>
```

#### Passed Example 12

Both this `p` and `span` elements have a single text node descendant, whose parent (the `span` element) is [wide enough][].

```html
<p style="font-size: 24px; letter-spacing: 2px !important">
<span style="letter-spacing: 3px !important;">
The toy brought back fond memories of being lost in the rain forest.
</span>
</p>
```

#### 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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…


```html
<p style="letter-spacing: 2.4px !important; font-size: 30px">
<!-- OK because too small but only whitespace -->
<span>&nbsp;</span>
<!-- OK because font-size has changed -->
<span style="font-size: 20px">The toy brought back fond memories of being lost in the rain forest.</span>
<!-- OK because letter-spacing has changed -->
<span style="letter-spacing: 3.6px !important"
>The toy brought back fond memories of being lost in the rain forest.</span
>
</p>
```

### Failed

#### Failed Example 1
Expand Down Expand Up @@ -253,14 +318,18 @@ The `style` attribute of this `p` element does not [declare][declared] the `lett
[computed]: https://www.w3.org/TR/css-cascade-4/#computed 'CSS Cascading and Inheritance Level 4 (Working draft) - Computed Values'
[declared]: https://www.w3.org/TR/css-cascade-4/#declared 'CSS Cascading and Inheritance Level 4 (Working draft) - Declared Values'
[font-size]: https://www.w3.org/TR/css-fonts-4/#propdef-font-size 'CSS Fonts Module Level 4 (Working draft) - Font size: the font-size property'
[html element]: #namespaced-element
[important]: https://www.w3.org/TR/css-cascade-4/#importance 'CSS Cascading and Inheritance Level 4 (Working draft) - Importance'
[inherited]: https://www.w3.org/TR/css-cascade-4/#inheriting 'CSS Cascading and Inheritance Level 4 (Working draft) - Inherited Values'
[letter-spacing]: https://www.w3.org/TR/css-text-3/#propdef-letter-spacing 'CSS Text Module Level 3 - Tracking: the letter-spacing property'
[normal]: https://www.w3.org/TR/css-cascade-4/#normal 'CSS Cascading and Inheritance Level 4 (Working draft) - Normal declarations'
[not important]: #24afc2:not-important 'The Not Important condition'
[sc1412]: https://www.w3.org/TR/WCAG21/#text-spacing 'Success Criterion 1.4.12 Text Spacing'
[sc148]: https://www.w3.org/TR/WCAG21/#visual-presentation 'Success Criterion 1.4.8 Visual Presentation'
[specificity]: https://www.w3.org/TR/selectors/#specificity 'CSS Selectors Level 4 (Working draft) - Specificity'
[used]: https://www.w3.org/TR/css-cascade-4/#used 'CSS Cascading and Inheritance Level 4 (Working draft) - Used Values'
[user origin]: https://www.w3.org/TR/css-cascade-4/#cascade-origin-user 'CSS Cascading and Inheritance Level 4 (Working draft) - Cascading Origins - User Origin'
[user agent origin]: https://www.w3.org/TR/css-cascade-4/#cascade-origin-ua 'CSS Cascading and Inheritance Level 4 (Working draft) - Cascading Origins - User Agent Origin'
[visible]: #visible 'Definition of visible'
[html element]: #namespaced-element
[whitespace]: #whitespace 'Definition of whitespace'
[wide enough]: #24afc2:wide-enough-cond 'The Wide Enough condition'
Loading