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

Dynamically defining exports from common #85

Closed

Conversation

aarongustafson
Copy link

@aarongustafson aarongustafson commented Jul 13, 2020

Related to #84, I had an idea that would not require a wholesale overhaul of the definition export. Curious for your thoughts @marcoscaceres @jnurthen

This commit:

  • Defines the terms to be exported in a variable definitions_to_export
  • Parses the "common" include using the oninclude hook and adds the data-export param to the existing dfn elements that match the values in that array.

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Aug 25, 2020, 8:06 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL


😭  Sorry, there was an error generating the HTML. Please report this issue!
�[36mSpecification: https://rawcdn.githack.com/w3c/accname/80f82400d396e8239b8632fdefc91dc433c3d069/index.html?isPreview=true&publishDate=2020-08-25�[39m
�[36mReSpec version: 25.6.0�[39m
�[36mFile a bug: https://github.com/w3c/respec/�[39m
�[36mError: Error: Evaluation failed: Timeout: document.respecIsReady didn't resolve in 29054ms.�[39m
�[36m    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:217:19)�[39m
�[36m    at runMicrotasks (<anonymous>)�[39m
�[36m    at processTicksAndRejections (internal/process/task_queues.js:97:5)�[39m
�[36m    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:106:16)�[39m
�[36m    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:128:12)�[39m
�[36m    at async fetchAndWrite (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:95:18)�[39m
�[36m    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:14:20)�[39m
�[36m    at async generate (/u/spec-generator/server.js:90:29)�[39m
�[36m�[39m

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@aarongustafson
Copy link
Author

If this approach is workable for you @jnurthen, it could be employed on any/all ARIA specs.

@michael-n-cooper michael-n-cooper changed the base branch from master to main August 25, 2020 20:06
@jnurthen
Copy link
Member

jnurthen commented Apr 1, 2021

we can't move to use data-export until w3c/respec#2825 is resolved

@marcoscaceres
Copy link
Member

we can't move to use data-export until w3c/respec#2825 is resolved

Just noting that the dfn panel is a consumer of the definitions, but it shouldn't block this.

@jnurthen
Copy link
Member

jnurthen commented Apr 7, 2021

@marcoscaceres I disagree. I cannot ship a spec with known accessibility errors in it.

@marcoscaceres
Copy link
Member

Ok, I'll look at adding a feature so you can disable the dfn panel in this spec. We can re-enable it once the aforementioned bug is fixed. Note, however, you are disadvantaging a larger set of users refusing to ship this even if the panel is a bit buggy - the panel still extremely useful for a significant number of users even if it can't currently be activated by keyboard.

@marcoscaceres
Copy link
Member

@jnurthen and I spoke on Slack about w3c/respec#2825 - Going to get some guidance on how best to fix w3c/respec#2825 from @jnurthen over on the other repo. I put up some suggestions.

@aarongustafson, I'll try to review the code + changes here too ASAP.

Comment on lines +144 to +161
var definitions_to_export = [ "accessible name", "accessible description" ];
function exportDefinitions( utils, content, url ){
var $dfnDiv = document.createElement("div"),
$dfns,
i;
$dfnDiv.innerHTML = content;
$dfns = $dfnDiv.querySelectorAll('dfn:not([data-export])');
i = $dfns.length;
while( i-- )
{
var text = $dfns[i].innerText.toLowerCase();
if ( definitions_to_export.indexOf( text ) > -1 )
{
$dfns[i].dataset.export = "";
}
}
return $dfnDiv.innerHTML;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var definitions_to_export = [ "accessible name", "accessible description" ];
function exportDefinitions( utils, content, url ){
var $dfnDiv = document.createElement("div"),
$dfns,
i;
$dfnDiv.innerHTML = content;
$dfns = $dfnDiv.querySelectorAll('dfn:not([data-export])');
i = $dfns.length;
while( i-- )
{
var text = $dfns[i].innerText.toLowerCase();
if ( definitions_to_export.indexOf( text ) > -1 )
{
$dfns[i].dataset.export = "";
}
}
return $dfnDiv.innerHTML;
}
function exportDefinitions(_, content) {
const dfnsToExport = ["accessible name", "accessible description"];
const temp = document.createElement("temp");
temp.innerHTML = content;
const dfns = [
...temp.querySelectorAll("dfn:not([data-export])"),
].filter(({ textContent: text }) =>
dfnsToExport.includes(text.toLowerCase().trim())
);
dfns.forEach(dfn => (dfn.dataset.export = ""));
return temp.innerHTML;
}

@marcoscaceres
Copy link
Member

I think this might be ok for a transitional step.

@@ -3,11 +3,8 @@
<head>
<title>Accessible Name and Description Computation 1.2</title>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type"/>
<script src="https://www.w3.org/Tools/respec/respec-w3c-common" class="remove" defer></script>
Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't touch any of these.

preProcess: [ linkCrossReferences ]
}
</script>
<script>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<script>
<script class="remove">

@jnurthen
Copy link
Member

Defintions have now been moved to their source specs. Most are defined in "wai-aria" and a few in "core-aam"

I think this can be closed.

@marcoscaceres
Copy link
Member

Thanks for the update @jnurthen!

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.

None yet

3 participants