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.

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

[Target size]: UA controlled rule #2182

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

Jym77
Copy link
Collaborator

@Jym77 Jym77 commented Apr 18, 2024

⚠️ THIS PR MAKES LITTLE SENSE IN ISOLATION ⚠️

Since #2167 is getting way too big, and according to CG call on April 11th, trying to extract the atomic rules to see if it can ease review.
This is an isolated rule for the larger "Target Size" rules. This rule alone makes little sense (as in, not checking anything valuable), please consider the bigger picture of #2167 while reviewing.

Closes issue(s):

  • N/A

Need for Call for Review:
This will require a 2 weeks Call for Review (new rule)


Pull Request Etiquette

When creating PR:

  • Make sure you're requesting to pull a branch (right side) to the develop branch (left side).
  • Make sure you do not remove the "How to Review and Approve" section in your pull request description

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.
  • Link the PR to any issue it solves. This will be done automatically by referencing the issue at the top of this comment in the indicated place.
  • Optionally request feedback from anyone in particular by assigning them as "Reviewers".

When merging a PR:

  • Close any issue that the PR resolves. This will happen automatically upon merging if the PR was correctly linked to the issue, e.g. by referencing the issue at the top of this comment.

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Call for Review period. In case of disagreement, the longer period wins.

@Jym77 Jym77 added Rule Use this label for a new rule that does not exist already reviewers wanted labels Apr 18, 2024
@Jym77 Jym77 self-assigned this Apr 18, 2024
@Jym77 Jym77 mentioned this pull request Apr 18, 2024
7 tasks
@Jym77 Jym77 marked this pull request as ready for review April 18, 2024 12:34

Directly setting the `height` or `width` CSS properties makes an element not User-Agent controlled. Changing the `max-height` or `min-width` properties can make it not User-Agent controlled, if the added constraint impacts the [computed][computed values] `height` or `width` properties (i.e., if the `height` would be larger than `max-height` and is restricted by it; or if the `width` would be smaller than `min-width` and is changed by it).

[cascaded value]: https://www.w3.org/TR/css-cascade-5/#cascade-value 'CSS definition of computed value'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[cascaded value]: https://www.w3.org/TR/css-cascade-5/#cascade-value 'CSS definition of computed value'
[cascaded value]: https://www.w3.org/TR/css-cascade-5/#cascaded 'CSS definition of cascaded value'

anchor seems not existing

Directly setting the `height` or `width` CSS properties makes an element not User-Agent controlled. Changing the `max-height` or `min-width` properties can make it not User-Agent controlled, if the added constraint impacts the [computed][computed values] `height` or `width` properties (i.e., if the `height` would be larger than `max-height` and is restricted by it; or if the `width` would be smaller than `min-width` and is changed by it).

[cascaded value]: https://www.w3.org/TR/css-cascade-5/#cascade-value 'CSS definition of computed value'
[computed values]: https://www.w3.org/TR/css-cascade-3/#computed 'CSS definition of Computed Value'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[computed values]: https://www.w3.org/TR/css-cascade-3/#computed 'CSS definition of Computed Value'
[computed values]: https://www.w3.org/TR/css-cascade-5/#computed 'CSS definition of Computed Value'

updated with css-cascade-5 new version

[cascaded value]: https://www.w3.org/TR/css-cascade-5/#cascade-value 'CSS definition of computed value'
[computed values]: https://www.w3.org/TR/css-cascade-3/#computed 'CSS definition of Computed Value'
[implicit role]: #implicit-role 'Definition of Implicit Role'
[origin]: https://drafts.csswg.org/css-cascade-5/#cascading-origins 'CSS definition of Cascading Origin'
Copy link
Collaborator

@giacomo-petri giacomo-petri Jun 20, 2024

Choose a reason for hiding this comment

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

Suggested change
[origin]: https://drafts.csswg.org/css-cascade-5/#cascading-origins 'CSS definition of Cascading Origin'
[origin]: https://www.w3.org/TR/css-cascade-5/#cascading-origins 'CSS definition of Cascading Origin'

updated with last candidate recommendation

- the element is [focusable][]; and
- the element [computed value][] of the `pointer-events` CSS property is `auto`; and
- through scrolling, it is possible to have parts of the element's [border box][] which intersect the viewport and are not entirely covered by the [border boxes][] of elements with greater [computed][computed value] `z-index` and a [computed][computed value] `pointer-events` of `auto`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a bullet point to clarify that the element must be visible (i.e., it is not set to display:none or visibility:hidden, it is not positioned out of the viewport such as left:-99999px, and its computed width and height are greater than 0)?

Without this clarification, it still makes sense theoretically, since the element can be targeted by a pointer event. However, if the element is not visible, this condition becomes practically useless as it cannot be targeted by a pointer device.

Or perhaps it is fine to keep it as is, but this statement should be defined within the applicability section for each rule.

- Not all HTML elements that match this definition can actually be targeted by a pointer event. For example, when the actual clickable area does not cover the full [border box][] and is entirely covered by other elements, or when the element has an event handler that does nothing. Elements that match this definition but cannot actually be targeted by pointer events likely fail [Success Criterion 2.5.6 Concurrent Input Mechanisms][sc256].

[border box]: https://www.w3.org/TR/css-box-3/#border-box 'CSS definition of Border Box'
[computed value]: https://www.w3.org/TR/css-cascade-3/#computed 'CSS definition of Computed Value'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[computed value]: https://www.w3.org/TR/css-cascade-3/#computed 'CSS definition of Computed Value'
[computed value]: https://www.w3.org/TR/css-cascade-5/#computed 'CSS definition of Computed Value'

updated with last definition

Comment on lines +118 to +119
height: 50px;
width: 500px;
Copy link
Collaborator

@giacomo-petri giacomo-petri Jun 20, 2024

Choose a reason for hiding this comment

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

Suggested change
height: 50px;
width: 500px;
min-height: 50px;
min-width: 500px;
color: transparent;

together with the CSS edit below, it ensures that if users zoom text only, the button is still covered by the red bordered box

<body>
<button onclick="alert('hello')">Say Hello</button>

<div class="cover bad highlight"></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div class="cover bad highlight"></div>
<div class="cover bad highlight">Say Hello</div>

## Assumptions

- This rule assumes that [focusable][] `widget` are effectively clickable. If a widget is [focusable][] without being clickable, it may fail this rule while [Success Criterion 2.5.5 Target Size (enhanced)][sc255] and [Success Criterion 2.5.8 Target Size (minimum)][sc258] are satisfied.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have secondary requirements, but should we clarify, once more, that "User Agent controlled components" are not required by WCAG, and the author can still customize them?

I’m concerned because the failing examples (especially the second one) currently meet 2.5.8, and I don’t want a distracted reader to think that custom controls are not allowed.

@Jym77
Copy link
Collaborator Author

Jym77 commented Aug 1, 2024

Failed examples need to also fail the composite rule so that implementation reports can be correctly generated for tools that do not report on every atomic rule.

Comment on lines +65 to +75
<style>
input[type='radio'] {
width: 1em;
height: 1em;
}
</style>
<fieldset>
<legend>Pick a color (required)</legend>
<label><input type="radio" name="color" value="blue" />Blue</label>
<label><input type="radio" name="color" value="yellow" />Yellow</label>
</fieldset>
Copy link
Collaborator

@tombrunet tombrunet Aug 1, 2024

Choose a reason for hiding this comment

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

Suggested change
<style>
input[type='radio'] {
width: 1em;
height: 1em;
}
</style>
<fieldset>
<legend>Pick a color (required)</legend>
<label><input type="radio" name="color" value="blue" />Blue</label>
<label><input type="radio" name="color" value="yellow" />Yellow</label>
</fieldset>
<style>
input[type='radio'] {
width: 1em;
height: 1em;
}
fieldset {
line-height: 1em;
}
</style>
<fieldset>
<legend>Pick a color (required)</legend>
<label><input type="radio" name="color" value="blue" />Blue</label><br/>
<label><input type="radio" name="color" value="yellow" />Yellow</label>
</fieldset>

Comment on lines +85 to +86
width: 35px;
height: 35px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
width: 35px;
height: 35px;
width: 23px;
height: 23px;


- the element is a [semantic `widget`][semantic role]; and
- the element is [focusable][]; and
- the element [computed value][] of the `pointer-events` CSS property is `auto`; and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to consider 'stroke' and 'fill'? These are valid for SVG elements. See https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events

Comment on lines +13 to +14
- the element is a [semantic `widget`][semantic role]; and
- the element is [focusable][]; and
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure both of these need to be true. At least one I would say.

Suggested change
- the element is a [semantic `widget`][semantic role]; and
- the element is [focusable][]; and
- the element is a [semantic `widget`][semantic role] or is [focusable][]; and

- the element is a [semantic `widget`][semantic role]; and
- the element is [focusable][]; and
- the element [computed value][] of the `pointer-events` CSS property is `auto`; and
- through scrolling, it is possible to have parts of the element's [border box][] which intersect the viewport and are not entirely covered by the [border boxes][] of elements with greater [computed][computed value] `z-index` and a [computed][computed value] `pointer-events` of `auto`.
Copy link
Member

Choose a reason for hiding this comment

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

This is tough to parse. How about:

Suggested change
- through scrolling, it is possible to have parts of the element's [border box][] which intersect the viewport and are not entirely covered by the [border boxes][] of elements with greater [computed][computed value] `z-index` and a [computed][computed value] `pointer-events` of `auto`.
- through scrolling, it is possible to have parts of the element's [border box][] appear within the viewport; and
- while scrolled within the viewport the element is not obscured by another element with a [computed][computed value] `pointer-events` of `auto`.

This definition tries to capture which HTML elements can actually react to pointer events. It is not possible to have an exact definition of these for two main reasons:

- Sometimes, the element that handles the event is not the element that appear to react to it, but an ancestor (or descendant) capturing the event during propagation or bubbling. In the most extreme case, the `body` element of a page could be the only one with an event handler, acting differently depending on where the event actually occurred. In such a case, a button would be perceived by users as something that can be targeted by a pointer event, while technically it is the `body` element which is targeted.
- It is not possible to query the list of event listeners on a given elements. Some User Agents offer way to monitor events fired at a given element, but none offer a way to query for event listeners. Additionally, an event listener might ultimately do nothing and thus, for users, the corresponding element wouldn't look like it can be targeted by pointer events (since it effectively wouldn't react to them).
Copy link
Member

Choose a reason for hiding this comment

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

That's not quite true. Most browser devtools panels can show this. There just isn't a built in DOM method.

Comment on lines +2 to +3
title: User-Agent Controlled Component
key: user-agent-controlled-component
Copy link
Member

Choose a reason for hiding this comment

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

Feels unnecessarily verbose. Was there a reason we can't use UA control that I missed?

Suggested change
title: User-Agent Controlled Component
key: user-agent-controlled-component
title: User-Agent Control
key: user-agent-control

Comment on lines +13 to +15
- the element has an [implicit role][] which is a [semantic `widget`][semantic role]; and
- the [computed values][] of the element's `height` and `width` CSS properties do not depend on content provided by the author; and
- the [computed values][] of the element's `height` and `width` CSS properties do not depend on any CSS property with a [cascaded value][] with "Author" [origin][].
Copy link
Member

Choose a reason for hiding this comment

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

This feels off. For starts, font-size is a CSS property often set by the author that changes the width and height.

I'm also not sure whether this then includes things that feel like they shouldn't. An input[type=text] element can be adjusted by the size attribute. I don't think that then creates an author-origin CSS property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewers wanted Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants