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

#33034: accessible description #33929

Merged
merged 11 commits into from
Jun 21, 2024
Merged

#33034: accessible description #33929

merged 11 commits into from
Jun 21, 2024

Conversation

estelle
Copy link
Member

@estelle estelle commented Jun 5, 2024

rewrote the glossary term.

fixes #33034

@estelle estelle requested a review from a team as a code owner June 5, 2024 00:47
@estelle estelle requested review from pepelsbey and removed request for a team June 5, 2024 00:47
@github-actions github-actions bot added Content:Glossary Glossary entries size/s [PR only] 6-50 LoC changed labels Jun 5, 2024
Copy link
Contributor

github-actions bot commented Jun 5, 2024

Preview URLs

External URLs (7)

URL: /en-US/docs/Glossary/Accessible_description
Title: Accessible description

(comment last updated: 2024-06-20 18:28:13)

Copy link
Contributor

@PassionPenguin PassionPenguin left a comment

Choose a reason for hiding this comment

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

@estelle for reader’s reference, will adding a link to related drafts/standards of many use?

estelle and others added 3 commits June 11, 2024 13:45
estelle and others added 2 commits June 11, 2024 14:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@estelle
Copy link
Member Author

estelle commented Jun 11, 2024

Thanks @PassionPenguin. I committed your suggestions, then edited the page.

Copy link
Contributor

@PassionPenguin PassionPenguin left a comment

Choose a reason for hiding this comment

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

i do not have the access to approve, and, the preview page seems not to be synced with the latest commit, but as for the markdown content itself, it’s now way better. merci beaucoup! 😃

@estelle
Copy link
Member Author

estelle commented Jun 12, 2024

@pepelsbey Bumping in case you missed this round-robin assignment. Thanks for your review.

Copy link
Contributor

@PassionPenguin PassionPenguin left a comment

Choose a reason for hiding this comment

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

@estelle maybe we can improve links (to keep consistent with accessiblity, accessibility_tree and accessibility_name) (#34095)

files/en-us/glossary/accessible_description/index.md Outdated Show resolved Hide resolved
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I was away from the review process last week.

I have concerns regarding the SVG section of this PR: it follows what’s written in the spec, which is not always aligned with browser implementations. For example, the xlink:title attribute is deprecated, the aria-label attribute is not mentioned, and the whole “contents of the <desc>” part is too complicated.

Maybe we should consider writing HTML and SVG computing accessibility names guides and linking them from the glossary. Otherwise, this entry becomes too big and detailed, which is unusual for the glossary.

@PassionPenguin
Copy link
Contributor

PassionPenguin commented Jun 13, 2024

Sorry for the delay, I was away from the review process last week.

I have concerns regarding the SVG section of this PR: it follows what’s written in the spec, which is not always aligned with browser implementations. For example, the xlink:title attribute is deprecated, the aria-label attribute is not mentioned, and the whole “contents of the <desc>” part is too complicated.

Maybe we should consider writing HTML and SVG computing accessibility names guides and linking them from the glossary. Otherwise, this entry becomes too big and detailed, which is unusual for the glossary.

thats why i did put an issue but didn't purposed a pr - spec are all in TR.

Separating the computation out of glossary could be a good idea to avoid bloated/overstuffed with detailed information, but we now do not even have articles on desc and name than these two glossary entries. Maybe we can create them in web accessibility section.

What's your opinion @estelle ?

Co-authored-by: Hoarfroster <hoarfroster@outlook.com>
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Jun 13, 2024
@github-actions github-actions bot added size/s [PR only] 6-50 LoC changed and removed size/m [PR only] 51-500 LoC changed labels Jun 13, 2024
@estelle
Copy link
Member Author

estelle commented Jun 13, 2024

All good points. I wasn't too keen on adding the SVG, but i do think we need to explain the computation. When i used to present on web forms in 2011 or so, the "wows" came from the new HTML input types, but how the accessible name was computed was the part of the talk that got the most photos of my slides taken and seemed to be of interest to every. (https://estelle.github.io/forms/#slide13)

So, I combined the two, since SVG is based on the HTML "with some differences," and added the SVG elements to step 3 in order of precedence. I think it makes the page less overwhelming while keeping the content. Does that work?

@PassionPenguin
Copy link
Contributor

All good points. I wasn't too keen on adding the SVG, but i do think we need to explain the computation. When i used to present on web forms in 2011 or so, the "wows" came from the new HTML input types, but how the accessible name was computed was the part of the talk that got the most photos of my slides taken and seemed to be of interest to every. (https://estelle.github.io/forms/#slide13)

So, I combined the two, since SVG is based on the HTML "with some differences," and added the SVG elements to step 3 in order of precedence. I think it makes the page less overwhelming while keeping the content. Does that work?

All good points. I wasn't too keen on adding the SVG, but i do think we need to explain the computation. When i used to present on web forms in 2011 or so, the "wows" came from the new HTML input types, but how the accessible name was computed was the part of the talk that got the most photos of my slides taken and seemed to be of interest to every. (https://estelle.github.io/forms/#slide13)

So, I combined the two, since SVG is based on the HTML "with some differences," and added the SVG elements to step 3 in order of precedence. I think it makes the page less overwhelming while keeping the content. Does that work?

LGTM. The computation is definitely needed (no matter detailed or brief it is) given NO computations are listed currently in the MDN documentations. How's your view on this update @pepelsbey ?

@estelle estelle requested a review from pepelsbey June 17, 2024 20:11
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

The merged computation section looks much better now. It would be better to have it in a reference somewhere else, but for a quick win, this might also work. I’d also suggest improving the initial definition to make it a bit shorter and more straightforward.

files/en-us/glossary/accessible_description/index.md Outdated Show resolved Hide resolved
files/en-us/glossary/accessible_description/index.md Outdated Show resolved Hide resolved
Co-authored-by: Vadim Makeev <hi@pepelsbey.dev>
@estelle estelle requested a review from pepelsbey June 20, 2024 18:26
@estelle
Copy link
Member Author

estelle commented Jun 20, 2024

Thanks @pepelsbey. Committed your suggested edits.

Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you :)

@estelle estelle merged commit 7076d72 into main Jun 21, 2024
9 checks passed
@estelle estelle deleted the estelle-patch-17 branch June 21, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Glossary Glossary entries size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexplained computation in glossary/accessible-name
3 participants