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

Update C42 to address issue 2433 #3231

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Update C42 to address issue 2433 #3231

wants to merge 25 commits into from

Conversation

fstrr
Copy link
Contributor

@fstrr fstrr commented Jun 6, 2023

closes #2433

  1. Added a technique on undersized targets relevant to Target Size (Minimum)
  2. Updated the existing examples, that are relevant to Target Size (Enhanced) to simplify the images and update the code
  3. Retitled C42 from "Using min-height and min-width to ensure sufficient target spacing" to "Using CSS to ensure sufficient target spacing"
  4. Updated the working examples to add the new undersized-targets example and updated examples from the Technique document
  5. Added a link to C42 from Target Size (Enhanced) and Target Size (Minimum).

techniques/css/C42.html Outdated Show resolved Hide resolved
Copy link
Contributor

@bruce-usab bruce-usab left a comment

Choose a reason for hiding this comment

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

Looks really good, but it is so different I would have expected a new technique (with a new number) rather than repurposing an obsolete technique.

- C42 is back to pertaining to just Target Size (Enhanced)
- Created new, currently un-numbered, technique for Target Size (Minimum)
- Added Related Techniques links to each technique
- Re-title image file names for clarity
@patrickhlauke
Copy link
Member

looking at https://raw.githack.com/w3c/wcag/issue-2433/techniques/css/C42.html and https://raw.githack.com/w3c/wcag/issue-2433/working-examples/css-sufficient-target-spacing/index.html ... I would question the use of rem there. This makes the size dependent on the root element's font size, which may be the default 16px ... or it may not. I'd say just go with explicit 44px.

more fundamentally, if the idea here is that we want to separate out AA and AAA, and the new https://raw.githack.com/w3c/wcag/issue-2433/techniques/css/adding-spacing-to-undersized-targets.html technique is meant to cover the AA scenario ... then I'd question what that first example in C42 is meant to convey? AAA doesn't have any kind of spacing clause, so what's the point of showing targets that are smaller than 44x44px but then saying that they have sufficient spacing? am I missing something?

- update Description to remove references to 24px
- update example to use px instead of rem
@fstrr
Copy link
Contributor Author

fstrr commented Jun 23, 2023

Thanks for the feedback. This is what happens when trying to edit documents and deal with school runs and sick kids. I'm glad you're as thorough as always with your reviews.

  1. I've replaced the description in C42 so it removes references to all the 24px content.
  2. I've updated the code in C42 to use px instead of rem
  3. I've updated the working examples for C42 to use px instead of rem

I think that resolves the issues you raised.

@patrickhlauke
Copy link
Member

But assuming C42 is intended to serve as technique for 2.5.5 Target Size (Enhanced) (AAA), it needs to remove the whole "spacing" angle (in the title, initial description, and all of the first example), as the AAA version can't be passed with spacing.

@patrickhlauke
Copy link
Member

patrickhlauke commented Jun 23, 2023

to be less cryptic/more specific about what I mean for C42 - if it's intended as a technique for 2.5.5 Target Size (Enhanced) only:

  • rename this to "Using CSS to ensure sufficient target size" (not "spacing")
  • change the actual description ... it's not about spacing, but pure size (since 2.5.5 doesn't have any exemption/stipulation for spacing)
  • remove first example completely
  • tweak the introductory paragraph for the other example (remove the "and not on the list items.")
  • change the caption for the figure from "Example of using CSS to ensure enough spacing within targets" to "Example of using CSS to ensure a sufficiently large target size" or similar

unless i'm misunderstanding what this PR is trying to achieve...is it trying to provide examples that cover both 2.5.5 and 2.5.8 Target Size (Minimum)? If so, that makes it weirdly confusing (and the test procedure at the end of C42 is still only about meeting 2.5.5). i thought the decision (from skimming meeting minutes etc) was to have two clearly separate techniques, one for 2.5.5, one for 2.5.8?

techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

@fstrr as I see a "Francis will figure it out" in the meeting minutes from yesterday, and the idea is indeed to make C42 explicitly about AAA, then #3231 (comment) should cover most of what should really change here (plus @dbjorge's comments above, which in part mirror what I noted in that comment)

understanding/22/target-size-minimum.html Outdated Show resolved Hide resolved
understanding/21/target-size-enhanced.html Outdated Show resolved Hide resolved
techniques/css/adding-spacing-to-undersized-targets.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
@fstrr
Copy link
Contributor Author

fstrr commented Jun 28, 2023

I think that's all the suggestions included. Relevant pages:

techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
techniques/css/C42.html Outdated Show resolved Hide resolved
Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

Perfect, thank you @fstrr

@fstrr
Copy link
Contributor Author

fstrr commented Jun 29, 2023

Thanks for your patience, @patrickhlauke

patrickhlauke added a commit that referenced this pull request Jul 13, 2023
to avoid clash with #3231
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks @fstrr! Looks good overall, a few minor/editoral suggestions inline

</section>
<section id="description">
<h2>Description</h2>
<p>The objective of this technique is to ensure that links in navigation menus have enough spacing around them so each link measures at least 24px width and 24px height. The aim is to provide an adequate target clearance to prevent accidental pointer activation of adjacent targets.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>The objective of this technique is to ensure that links in navigation menus have enough spacing around them so each link measures at least 24px width and 24px height. The aim is to provide an adequate target clearance to prevent accidental pointer activation of adjacent targets.</p>
<p>The objective of this technique is to ensure that links in navigation menus have enough spacing around them to meet the requirements of the spacing exception in Success Criterion 2.5.8, Target Size (Minimum). The aim is to provide an adequate target clearance to prevent accidental pointer activation of adjacent targets.</p>


<section class="test-procedure">
<h3>Procedure</h3>
<p>For each undersized target that has no equivalent, that isn't inline or otherwise constrained by the line-height of non-target text, that hasn't had its size modified by the author, and whose presentation isn't essential:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

The term inline has a specific meaning in CSS that is different from the meaning established by the SC's Inline exception; since this is a CSS example, I think it'd be good to avoid the ambiguity by spelling out the SC meaning.

Suggested change
<p>For each undersized target that has no equivalent, that isn't inline or otherwise constrained by the line-height of non-target text, that hasn't had its size modified by the author, and whose presentation isn't essential:</p>
<p>For each undersized target that has no equivalent, that isn't in a sentence or otherwise constrained by the line-height of non-target text, that hasn't had its size modified by the author, and whose presentation isn't essential:</p>

</ul>
</section>
</section>
<section id="related">
<h2>Related Techniques</h2>
<ul>
<li><a href="adding-spacing-to-undersized-targets.html">CXXXXXX</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<li><a href="adding-spacing-to-undersized-targets.html">CXXXXXX</a></li>
<li><a href="C44">C44</a></li>

<section id="related">
<h2>Related Techniques</h2>
<ul>
<li><a href="C42.html">Using CSS to ensure sufficient target size</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other techniques:

Suggested change
<li><a href="C42.html">Using CSS to ensure sufficient target size</a></li>
<li><a href="C42">C42</a></li>

AbdullahWins

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C42 good for 2.5.5 Target Size (Enhanced) not as example for 2.5.8 Target Size (Minimum)
7 participants