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

Mapping tables script is creating id="undefined" on summary elements #7

Open
AmeliaBR opened this issue Apr 2, 2018 · 17 comments
Open
Assignees

Comments

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Apr 2, 2018

A bug in the script that converts the role mapping tables into summary/details means that the id of the summary elements are all being set to undefined.

In addition to being just invalid HTML, this breaks links, since those were the id that are used as hash references (and they need to be the id used as hash references, so that the browser will auto-expand the corresponding details when the summary is targetted). This makes it a blocking issue for SVG-AAM republication.

My best guess is it was introduced here:
bc45948
@jasonkiss, are you able to take a look?

Tracking issue for Core-AAM: w3c/core-aam#5

I'll also make a tracking issue in SVG-AAM, since I acquired the bug when I synced all the common files last week. HTML-AAM isn't currently affected. I'm not sure if there are any other specs that use the script.

@jasonkiss
Copy link
Contributor

Only just took a quick peek, but from what I can tell, HTML-AAM and CORE-AAM and SVG-AAM are all using the identical mapping-tables.js (albeit at different locations).

But I also noticed that something appears to be stripping the id attributes from the tr elements in the mapping tables. Not sure where/when in the process this is happening, but the mapping-tables script relies on those tr elements having id attribute values, which the raw index.html files for all AAM specs have prior to processing.

@AmeliaBR
Copy link
Contributor Author

AmeliaBR commented Apr 9, 2018

Added a tracking link in Graphics-AAM: this affects the CR build published last month.

@AmeliaBR
Copy link
Contributor Author

@jasonkiss Are you able to review PR #9 by @kevinpeno, to see if it takes care of the issue without introducing any new ones?

@michael-n-cooper
Copy link
Member

ARIA Editors discussion

@jasonkiss
Copy link
Contributor

I've looked at PR #9 and don't think it addresses this issue.

One thing I don't understand is how the respec snapshot contains the mapping tables modified by the mapping-tables script, but not the details/summary elements created by the same script.

@kevinpeno
Copy link
Contributor

I've tried to review this again, but I am no longer able to duplicate locally when testing against the w3c/core-aam repo's master branch.

@jasonkiss
Copy link
Contributor

In the mapping-tables.js script, there is the following code at the end:

if (respecEvents) {
		// Fix the scroll-to-fragID:
		// - if running with ReSpec, do not invoke the mapping tables script until
		//   ReSpec executes its own scroll-to-fragID.
		// - if running on a published document (no ReSpec), invoke the mapping tables
		//   script on document ready.
		respecEvents.sub("start", function (details) {
			if (details === "core/location-hash") {
				mappingTables();
			}
		});
		// Subscribe to ReSpec "save" message to set the mapping tables to
		// view-as-single-table state.
		respecEvents.sub("save", function (details) {
			mappingTableInfos.forEach(function (item) {
				viewAsSingleTable (item);
			});
		});
	} else {
		mappingTables();
	}

However, while I can confirm that respecEvents is being passed through, it's not clear to me that the "save" event is registering and triggering the viewAsSingleTable() prior to exporting the static respec snapshot. If that did work, it would solve the issue. Do we know if this is the right approach with RespecEvents?

@AmeliaBR
Copy link
Contributor Author

One thing I don't understand is how the respec snapshot contains the mapping tables modified by the mapping-tables script, but not the details/summary elements created by the same script.

The details elements have the removeOnSave class, which must be stripping them out successfully. But the id values aren't being reinstated on the table elements. So maybe the remove is happening before the attempt to switch back to table view?

@jasonkiss
Copy link
Contributor

The details elements have the removeOnSave class

Yes, thanks! I should have noticed that.

But the id values aren't being reinstated on the table elements. So maybe the remove is happening before the attempt to switch back to table view?

That could be. But I'm unable to confirm via alert or console.log that the "save" event is even getting through to the mapping-tables script.

If we can't get the "save" event and viewAsSingleTable() switch to happen as intended, another option is to not remove the details/summary and modify the script accordingly so that it does not try to recreate the details/summary if they already exist.

@jnurthen
Copy link
Member

jnurthen commented Nov 10, 2018

Could we use the "end" event here? In my brief testing it appears to work....

@ZoeBijl
Copy link

ZoeBijl commented Oct 7, 2019

@jnurthen is this still on your todo or can I take over?

@jnurthen
Copy link
Member

jnurthen commented Oct 7, 2019 via email

@carmacleod
Copy link
Contributor

Thank-you, @ZoeBijl !
I believe this would also fix w3c/html-aam#255

@carmacleod
Copy link
Contributor

@ZoeBijl @jnurthen I could really use a fix for this, so that whatwg/html#3282 can be resolved with working links into the published version of the HTML-AAM spec instead of a temporary work-around of linking into the editor's draft.

If I understand correctly from the thread on the ARIA editors mailing list, this is at present a manual process that is prone to error.

The following info may be helpful:
From @michael-n-cooper:

As far as I can tell when I looked at this, it's a "feature" not a "bug"
that the script removes the IDs, as part of changing the table to a set
of twistie ties. I don't know if it's inherently necessary to do it that
way, but if it is, the only script fix would be to get Respec to disable
that script or activate the "view as table" buttons first when exporting
a snapshot.

From @jasonkiss:

the "view as table" prior to respec snapshot is a solution because the mapping-tables script rewrites the table content as a bunch of details/summary elements, one for each row, and then hides the table. In that process, the IDs on the table rows are stored in an array, applied to the details elements, and removed from the table rows. If the respec snapshot is taken in this view, then the table rows have no IDs, and the mapping-tables script works from the assumption that the initial HTML is a table with IDs on the TRs, so when it tries to work on this snapshot DOM, it finds no IDs on the table TRs, hence the "undefined”.

@marcoscaceres has offered ReSpec help:

if there is something we should be doing in ReSpec core, please let me know.

@ZoeBijl
Copy link

ZoeBijl commented Feb 27, 2020

I have not forgotten this one. Will fix after I get aria-practices #1228 fixed.

@carmacleod
Copy link
Contributor

Hi @ZoeBijl!

Note that what I thought was a "temporary work-around of linking into the editor's draft" for the fix for whatwg/html#3282 turned out to be exactly what the HTML editors wanted. Linking to the editor's draft fits the "Living Standard" model. :)

So, while id="undefined" is still a problem in the published version of the HTML-AAM spec, I am no longer waiting for it. :)

Thanks!

@scottaohara
Copy link
Member

scottaohara commented Mar 21, 2021

HTML AAM can't properly link to Core AAM due to this as well.

related: seems like it might be worth dropping jquery.details.min.js as well, unless we need to support IE11?

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 a pull request may close this issue.

8 participants