-
Notifications
You must be signed in to change notification settings - Fork 172
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
fix: Links with unique targets should have unique labels #307
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Requested reviews from:
|
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.
Cool! I think this checks out, pretty straightforward modification.
Funny in that we just cut another release, but we can go through this process once more ;).
I had a shower thought: In the future, we may wish to internationalize the aria value. what if we store the Let's say… |
a) wouldn’t a postprocessor also be able to search for the English language? |
👍🏼 as one of the people who previously modified. This looks good to me, but I am also not super familiar with C. Thanks for making this change though! |
@wooorm: I believe @phillmv was suggesting that this project should output both readable output and the index. That suggestion makes sense to me, because it’s more robust than relying on the postprocessor to extract the index from an English phrase. For example, if, in the future, we decide to change “Back to reference 1-2” to “Back to location 1–2”, a postprocessor using a |
@@ -63,10 +63,14 @@ static bool S_put_footnote_backref(cmark_html_renderer *renderer, cmark_strbuf * | |||
if (renderer->written_footnote_ix >= renderer->footnote_ix) | |||
return false; | |||
renderer->written_footnote_ix = renderer->footnote_ix; | |||
char m[32]; | |||
snprintf(m, sizeof(m), "%d", renderer->written_footnote_ix); |
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 as a note, my concern would be about snprintf implementations that don't nul terminate properly. If it were possible to get a 32byte width integer value out of the backref indexes (highly unlikely imo), we are not guaranteed a nul terminated string in m, which may lead to memory info leaks as cmark_strbuf_puts is strlen based (i.e. it will think len is 32 + whatever offset the first nul byte is located in adjacent memory). AFAICT everything that we build against in our test matrix is C99 conformant (but e.g. old visual studio versions pre-2015 were not iirc). /cc @kevinbackhouse for thoughts.
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.
If we wanted to be abundantly careful re: C99 compliance we could maybe do something like:
snprintf(m, sizeof(m), "%d", renderer->written_footnote_ix); | |
char m[32]; | |
memset(m, 0, sizeof(m)); | |
snprintf(m, sizeof(m)-1, "%d", renderer->written_footnote_ix); |
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.
As per @kevinbackhouse %d is not able to print an integer of that width, so this should be fine.
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.
Thanks for raising this, @anticomputer! To clarify, by “this should be fine”, did you mean that the code in this PR is fine as-is, or that the snippet you provided above sufficiently addresses the issue you noted?
I’m happy to go with whatever you recommend. Should we change this elsewhere? I was following the examples below, and I don’t know C or this codebase well enough to understand if these are categorically different in some way:
Lines 74 to 75 in eb32891
char n[32]; | |
snprintf(n, sizeof(n), "%d", i); |
and
Lines 432 to 433 in eb32891
char n[32]; | |
snprintf(n, sizeof(n), "%d", node->footnote.ref_ix); |
Added in 388ddec. 👍 |
* GH previously generated HTML for backreferences to repeated references that was not accessible, as it failed [WCAG 2.1 SC 2.4.4 — Link Purpose (In Context)](https://www.w3.org/TR/WCAG21/#link-purpose-in-context) * GH changed the text content they use in their backreferences from `Back to content` to `Back to reference i`, where `i` is either `x` or `x-y`, of which `x` is the reference index, and `y` the reference index This commit changes all HTML output for users that relied on the defaults, so that it matches GH again, exactly. The default handling is exposed as `defaultBackLabel`. Users who set `backLabel` are not affected. But these users can now provide a function instead of a `string`, to also solve the WCAG issue. The type for this function is exposed as `BackLabelTemplate`. Related-to: github/cmark-gfm#307.
* GH previously generated HTML for backreferences to repeated references that was not accessible, as it failed [WCAG 2.1 SC 2.4.4 — Link Purpose (In Context)](https://www.w3.org/TR/WCAG21/#link-purpose-in-context) * GH changed the text content they use in their backreferences from `Back to content` to `Back to reference i`, where `i` is either `x` or `x-y`, of which `x` is the reference index, and `y` the rereference index This commit changes all HTML output for users that relied on the defaults, so that it matches GH again, exactly. The default handling is exposed as `defaultFootnoteBackLabel`. Users who set `footnoteBackLabel` are not affected. But these users can now provide a function instead of a `string`, to also solve the WCAG issue. The type for this function is exposed as `FootnoteBackLabelTemplate`. Additionally, you can now pass `footnoteBackContent` to set the *content* of the backreference. Related-to: github/cmark-gfm#307. Closes remarkjs/remark-rehype#32.
Follow-up to #234
Problem
Currently, every footnote backref has the same
aria-label
: “Back to content”. However, if you click several of these links, you’ll end up in several different places in the main content. “Back to content” does not sufficiently describe the links’ purpose, resulting in a failure of WCAG 2.1 SC 2.4.4 — Link Purpose (In Context).@jscholes noted this during the January 25th, 2023 Accessibility Team Office Hours (34:29 in the recording (only accessible to Hubbers)):
I volunteered to follow up, in https://github.com/github/accessibility/issues/2740#issuecomment-1403828949 (only accessible to Hubbers).
Solution
This PR updated footnote backref
aria-label
’s to uniquely match locations in the main content:aria-label
: “Back to reference 1”.aria-label
: “Back to reference 2”.aria-label
: “Back to reference 1-2”.[^note3]
), the corresponding link back is given thearia-label
: “Back to reference 3”.What should reviewers focus on?
There’s a 50-50 chance I messed up the named tag logic; I wouldn’t be surprised if (considering the example above) I’ve inadvertently given theUpdate: Fixed via 1f30fbb and 4cc5541.aria-label
: “Back to reference note3”. I’m hoping the tests run in CI and flag this, if so.