Open AmeliaBR opened 6 years ago
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.
Added a tracking link in Graphics-AAM: this affects the CR build published last month.
@jasonkiss Are you able to review PR #9 by @kevinpeno, to see if it takes care of the issue without introducing any new ones?
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 table
s modified by the mapping-tables script, but not the details
/summary
elements created by the same script.
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.
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?
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?
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.
Could we use the "end" event here? In my brief testing it appears to work....
@jnurthen is this still on your todo or can I take over?
Go for it.
Thank-you, @ZoeBijl ! I believe this would also fix https://github.com/w3c/html-aam/issues/255
@ZoeBijl @jnurthen I could really use a fix for this, so that https://github.com/whatwg/html/issues/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.
I have not forgotten this one. Will fix after I get aria-practices #1228 fixed.
Hi @ZoeBijl!
Note that what I thought was a "temporary work-around of linking into the editor's draft" for the fix for https://github.com/whatwg/html/issues/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!
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?
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 toundefined
.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 theid
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: https://github.com/w3c/aria-common/commit/bc4594887ba81270b2f535bfe00f5c50655b58e2 @jasonkiss, are you able to take a look?
Tracking issue for Core-AAM: https://github.com/w3c/core-aam/issues/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.