-
Notifications
You must be signed in to change notification settings - Fork 70
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 Enhanced" - atomic rule #2279
base: develop
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,819 @@ | |||
--- | |||
id: gi8qkf |
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.
For first pass of review, please focus on global structure to se whether this is a good structure for the rule. We'll discuss details of the definitions later.
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.
The general structure looks good to me. I am interested in the definition of "inline". I look forward to that part.
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.
Great work so far. Looking forward to moving this one forward.
title: Clickable area | ||
key: clickable-area | ||
unambiguous: true | ||
objective: false |
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.
Why is this definition not objective?
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.
Because I feel that "topmost event target" is not really objective.
- the element is [rendered on a line]; or | ||
- the element is [User Agent controlled][user agent controlled component]; or | ||
- the element has [essential size][]. |
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.
Just wondering if these should be in the expectation?
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.
The full point of #2214 and the CG discussion was to have them as Inapplicable 🤷
</body> | ||
``` | ||
|
||
### Inapplicable |
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.
Shouldn't we have one with a clickable area that is not a semantic widget?
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.
🤔 Wouldn't that be bad for 1.3.1?
a <span onclick="alert('Hello')">Say hello</span>
would fail 1.3.1. We can have one (with associated caveats), but I'm afraid it would cause problems for tools that do not have the same separation as ACT rules and would (rightfully) fail it.
Co-authored-by: Carlos Duarte <[email protected]> Co-authored-by: Kevin White <[email protected]>
✅ Deploy Preview for act-rules ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -0,0 +1,37 @@ | |||
--- | |||
title: Observed as a pointer events target |
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.
Could the words "observed as" be cut out?
I know you added those words recently, so I assume you see considerable value in them. Still I must ask this, because when I first read those words, it made me wonder: is there another term called "pointer events target" that is different from "observed as a pointer events target"? If so, what does the venn diagram of these two sets look like? After I read the background and searched elsewhere for a minute, I arrived at the answers "no, or at least: not officially" and "n/a". If I'm the only reader that will go through that thought process: then it doesn't matter. If I'm not: then we have a cognitive load question here for future readers.
The background is essential commentary, but I'm not sure that it's essential to have it in the wording of the term itself. I figure most conversations about target size will just talk about "pointer event targets" and won't need to get into the details.
To summarize: the value of having "observed as" in the wording of the term itself is, IMO, outweighed by the cost of the added cognitive load.
@@ -0,0 +1,19 @@ | |||
--- | |||
title: In a Block of Text |
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 was going to look at the examples of this PR via the new "deploy preview" functionality, which is here. When I clicked on the "Open in a new tab" links for the examples, they didn't work, because they're pointing to w3.org rather than deploy-preview-2279--act-rules.netlify.app. Is this a known limitation? If so, can I help fix it?
New take at "Target Size" rule, as an automated rule with definitions, following CG decision.
For first pass, please focus reviews on the overall structure rather than the details of each definition 🙂
Closes issue(s):
Need for Call for Review:
This will require a 2 weeks Call for Review (new rule)
Pull Request Etiquette
When creating PR:
develop
branch (left side).After creating PR:
Rule
,Definition
orChore
.When merging a PR:
How to Review And Approve