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

Editorial: Replace mapping table and summary/details with headings #180

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

spectranaut
Copy link
Contributor

@spectranaut spectranaut commented Jun 16, 2023

Githack link, as PR Preview doesn't include styles: https://raw.githack.com/w3c/core-aam/remove-tables/index.html#mapping_role


Preview | Diff

index.html Outdated Show resolved Hide resolved
@pkra
Copy link
Member

pkra commented Jun 19, 2023

My browser is hanging when opening up the larger commit so I can't leave inline comments.

Here are some things I noticed so far

  • the visual styles are mostly gone (summary/details, the table inside). Should we try to recover them?
  • the content in the summary elements is repeated in a table caption.
    • Is this intentional? (e.g., to avoid links in summary)
    • Also, local references in those table captions appear to be broken

@spectranaut @jnurthen I'm not sure how to make sure that nothing was accidentally dropped. Do you have any thoughts on that?

@spectranaut
Copy link
Contributor Author

Here are some things I noticed so far

* the visual styles are mostly gone (summary/details, the table inside). Should we try to recover them?

Do you mean in the PR Preview? The styles have always been broken here, unfortunately. You can download and look at it locally or use githack: https://raw.githack.com/w3c/core-aam/remove-tables/index.html#mapping_role

I wonder if there is a way to fix this.

* the content in the summary elements is repeated in a table caption.
  * Is this intentional? (e.g., to avoid links in summary)

This is how it was before, but we can change it. The "repeat" was a link to ARIA, but now it seems broken, as you point out below.

  * Also, local references in those table captions appear to be broken

I'll fix this, it's weird though.. those used to be links back to the ARIA spec. But even in the editors draft they link just back to core-aam, I'm not sure how or when this changed.

@spectranaut @jnurthen I'm not sure how to make sure that nothing was accidentally dropped. Do you have any thoughts on that?

I wrote a little script to do the translation locally, I'm pretty confident I didn't drop anything! I'd guess we don't have to worry about that.

@pkra
Copy link
Member

pkra commented Jun 20, 2023

Thanks for clarifying, @spectranaut! (Especially kindly forgiving me for forgetting about the CSS issue).

This looks good to me then.

@pkra
Copy link
Member

pkra commented Jun 20, 2023

FYI the PR preview issue seems to be tobie/pr-preview#87

@pkra
Copy link
Member

pkra commented Jun 26, 2023

As per ARIA Editors meeting 2023-06-26, @scottaohara wants to coordinate with @spectranaut on this.

@scottaohara
Copy link
Member

spent time checking this out and while i had mentioned during the editor's call that maybe we could revise the presentation/markup for this component, I'm going to retract those ideas for now. No reason to hold this up further, and i'm doubtful the benefit of changing this up further at this point.

i'm assuming the broken links to the ARIA spec will be fixed once this gets merged? The links are setup exactly the same between the published version and this, so I can only assume it's just some problem with respec not building properly?

@pkra
Copy link
Member

pkra commented Jul 10, 2023

As per ARIA Editors meeting 2023-07-10, @spectranaut will simplify further, dropping summary/details, introducing headings etc.

@spectranaut
Copy link
Contributor Author

The HTML is a little wacky, I'll fix before landing, but @scottaohara and @pkra and @jnurthen what do you think of this radical change we discussed: https://raw.githack.com/w3c/core-aam/remove-tables/index.html#mapping_role

@scottaohara
Copy link
Member

i think it's an improvement.

tbh i'm going back and forth on whether the individual roles/attributes should take part in the TOC... but it would definitely help with the easier permalinking

@pkra
Copy link
Member

pkra commented Jul 29, 2023

I'll second @scottaohara -- looks much better to me, too!

Could we drop the table captions as well if we linked the terms in the headings? For roles, we could alternatively just link the computed role entry but we don't have that for states and properties (though I suppose that might happen if we had synonymous properties some day).

@spectranaut
Copy link
Contributor Author

Could we drop the table captions as well if we linked the terms in the headings? For roles, we could alternatively just link the computed role entry but we don't have that for states and properties (though I suppose that might happen if we had synonymous properties some day).

I was also trying to think of a way to drop the headings. Maybe I'll just add an extra row to all the tables and say "WAI-ARIA reference" in the first column and the relevant linked role or attribute in the second?

@jcsteh
Copy link

jcsteh commented Aug 8, 2023

  • As an implementer, being able to easily link to the individual mappings is going to be awesome. Thank you.
  • This also works just fine for me as a screen reader user.
  • Removing summary/details does mean this takes noticeably longer to load with a screen reader. I'm not sure about the load time without. That's probably not a big concern, but I thought it worth noting. It also probably means I have more a11y engine optimisation to do. :)

@jcsteh
Copy link

jcsteh commented Aug 8, 2023

Maybe I'll just add an extra row to all the tables and say "WAI-ARIA reference" in the first column and the relevant linked role or attribute in the second?

That's arguably clearer anyway. Right now, unless you know how the spec is structured, you might not necessarily know what that link is for, since it effectively duplicates the heading label.

@spectranaut spectranaut changed the title Editorial: Replace mapping table with summary/details only Editorial: Replace mapping table and summary/details with headings Aug 18, 2023
Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

@spectranaut i think this is good to go.

optional change would be to update the spec to not include the individual role/attribute names in the TOC. but that doesn't need to be part of this change, if it's done at all

@spectranaut spectranaut merged commit 356b21f into main Aug 21, 2023
2 checks passed
@spectranaut spectranaut deleted the remove-tables branch August 21, 2023 18:34
github-actions bot added a commit that referenced this pull request Aug 21, 2023
…ings (#180)

SHA: 356b21f
Reason: push, by spectranaut

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants