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

Spec format: Add anchors/link to table rows? #102

Closed
spectranaut opened this issue Nov 29, 2021 · 16 comments
Closed

Spec format: Add anchors/link to table rows? #102

spectranaut opened this issue Nov 29, 2021 · 16 comments
Assignees

Comments

@spectranaut
Copy link
Contributor

I have wanted to link to specific items in the Role Mapping, State and Property Mapping Table or State and Property Change Events multiple times. But, it's not clear how to do this, because there is a table AND a list of summary items in the DOM, one of which is displayed and one of which is hidden, so you can't have the same id on same content in both places.

Has anyone thought about how they might do this before? I'm happy to look into it and suggest a solution but I just wanted to see if anyone already had ideas.

I'm also curious, why do we have both the table and summary view of these items? Do we know that both are used, or whether one is better?

@jnurthen
Copy link
Member

Something is broken in aria-common/script/mapping-tables.js - if you could take a look and work out what is wrong it would unbreak a lot of specs which seem broken right now.

What is meant to happen (I think) is that the id's which are on the table version are meant to get used on the summary version and the id's on the table should then be removed.
IMO The summary version seems to be the best of the 2 to link to. If someone wants the tabular version then they are looking at a larger number of entries together so linking to an individual row doesn't sounds useful.

@spectranaut
Copy link
Contributor Author

Oh interesting.. and yes I can look into it! How many specs use aria-common/script/mapping-tables.js? Do they all separately need a fix or is there an upstream somewhere?

@jnurthen
Copy link
Member

All the AAMs (core-aam, html-aam, dpub-aam etc.) use it and it lives in the aria-common repository. I'd recommend testing fixes in core-aam and then checking the changes into aria-common.

@jnurthen
Copy link
Member

jnurthen commented Dec 1, 2021

a few more details:

  • The source document has the ids on the table
  • the script creates an array mappingTableInfos which contains objects for each of the tables with the ids
  • The script then assigns those ids to the summary items, removes from the table and hides the table
  • The button can toggle back and forth between the 2 views.
  • The summary view has removeonsave so doesn't end up in the final version.

In the published document similar things happen but the problem is that the ids are no longer on the table so the mappingTableInfos array is empty, then when the table gets rendered.

IMO we need to either

  • find a way to have respec ensure we are on the table view when exporting (with ids) - we used to have this with the save event but that no longer works
  • remove the removeonsave from the summary and have the script build the array from whichever of the 2 options (table or summary) have ids.

@marcoscaceres
Copy link
Member

It's correct that ReSpec no longer publishes its internal events.

We have a few options:

  1. I can add a special beforeSave property to the config to allow you to modify the source of the generated document.
  2. Instead of removing the ids, you convert them to "data-id" (or something similar), and then you can include a <script> that runs onload that does the conversion you need.

Either option would give you the ability to manipulate the document... but option 1 might be nice is the end result means no additional JS ends up on static document.

Let me know how you want to proceed... option 1 should be pretty easy to code up, but I'm traveling this week so would be something I could may do next week (unless someone else wants to have a go at coding it).

@spectranaut
Copy link
Contributor Author

I think I like option 1 more, @marcoscaceres, if you would like to implement it!

Also I'm realizing my concern for the lack of ids was actually part of my wish for a self link by each summary element -- @jnurthen what do you think about a self self-links like the ones to the left of the section headers? What if I added one to each role?

Options:

  • I could make the text of the summary a "self link" but it might look a bit odd: You will see "alert" which is a self link, followed immediately in the details by "alert" which is a link to the ARIA spec.
  • I could use that "self link" icon to the left of the text in the summary, similar to the section headers, or floated to the right of the summary.

@scottaohara
Copy link
Member

scottaohara commented Dec 3, 2021

Making the text within the summary a link, or including a link in the summary will introduce the peculiar nature of summary allowing links to our specifications (per all our recent talking on the ARIA WG about this exact problem). My vote would be for the self link to not be included in the summary element.

@jnurthen
Copy link
Member

jnurthen commented Dec 3, 2021

@marcoscaceres @spectranaut I see benefits to both options:

  1. Less code change for us. Keeps the published spec leaner by not repeating all the information twice in the static document.
  2. Would allow less scripting to run when simply viewing a spec as we could save the document in the state that most viewers actually read it (summary/details).

@spectranaut
Copy link
Contributor Author

Making the text within the summary a link, or including a link in the summary will introduce the peculiar nature of summary allowing links to our specifications (per all our recent talking on the ARIA WG about this exact problem). My vote would be for the self link to not be included in the summary element.

@scottaohara yeah it is funny that we have been talking about it so much, but I thought the idea was that people did this so we need to support it. But if screen readers can't handle it right now then it doesn't make sense to put the link in the summary.

What about a self link to the right or left of the summary? :)

@scottaohara
Copy link
Member

scottaohara commented Dec 3, 2021

yes, left or right of the summary would be fine imo.

edit: and to clarify, yes, we definitely do want to figure out how to support this in some way. but since it has known issues now, i'm just saying i'd rather us not do something we know to be quirky until we can find a fix for said quirk.

@spectranaut
Copy link
Contributor Author

Hey all I just thought I'd take a look at this and implement on of the solutions discussed above -- but I can't replicate the error locally. The github page's published version of this doc does not have the ids on the summary table, but when I open the file locally I see the ids. I looked at the publishing through github actions, and downloaded respec, and used the respec commandline tool (tools/respec2html.js) to create the published version, and it also has the ids. Am I missing something in trying to replicate the gh-pages build? Or is it a weird race condition?

Also, how does removeOnSave work? Under what condition are those things removed? I wanted to be able to test that scenario too.

@jnurthen
Copy link
Member

Also, how does removeOnSave work? Under what condition are those things removed? I wanted to be able to test that scenario too.

I think everything with removeOnSave should be removed from the saved document

When I looked at this previously the issue was that mapping-tables.js moves the ids from the table to the summary/details structure. I believe that when @michael-n-cooper creates new versions of this document he has previously switched back to the tabular version before saving the document from the respec pill (at the top right), but if he forgets then the ids don't get saved (as summary/details has removeOnSave AND the script expects the ids on the table anyway)

We need to fix this as we need to get to the point where we can auto-publish this document rather than relying on manual steps for publication. I believe I coded something that fixed this in a5323e4 but didn't get round to fully testing it (and moving the code to aria-common where it should be)

@spectranaut
Copy link
Contributor Author

It's funny @jnurthen I did almost the exact change when looking around at the code yesterday, so i just opened a PR with my branch (https://github.com/w3c/core-aam/pull/105/files) so we can discuss further there?

@benbeaudry
Copy link
Contributor

Is it possible that this issue is still happening? I don't see an easy way of creating an URL that contains the id of the fragment I'd like the page to scroll to upon loading. I can do it by going to the DevTools, getting the id of the element I want to scroll to and then manually modifying the URL to include it, but that's not really user friendly 🥲

More specifically, I'd like to be able to copy URL with fragment for specific attributes (e.g.: https://www.w3.org/TR/core-aam-1.2/#ariaErrorMessage), the same way I can do it for sections such as https://www.w3.org/TR/core-aam-1.2/#mapping_state-property.

From the issue title and description, it sounds like this is the same issue... but feel free to let me know if I'm mistaken!

@jnurthen
Copy link
Member

Not the same issue but I have filed this against aria-common

@spectranaut
Copy link
Contributor Author

The removal of the giant mapping table/summary details has achieved this!

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

No branches or pull requests

5 participants