refactor(aria.js): first steps #106
Conversation
A simplistic test to test aria.js output. Runs respec before and after copying aria.js changes. NOTE: Assumes there's a copy of w3c/aria in ../aria/
A simplistic first step. Running test.sh was clean.
test.sh run was 'clean'.
test.sh runs clean.
Revises pdef/sdef HTML-ification. Switches to template string. Moves this until after populating propList for future re-organization. test.sh run is clean.
Revises pdef/sdef populating globalSP. DRYs else-if clauses test.sh run is clean.
Simplifies first use of "sortedList" by a) realizing that propList is already sorted since the spec is. b) iterating over propList's values test.sh run is clean.
Simplifies second (and last) use of "sortedList" by a) realizing that globalSP is already sorted since the spec is. b) iterating over globalSP's values test.sh run is clean.
Revises "proplist to HTML" aka generating the index of states and properties. * replaces loop of string concatenation with map and join * switch to template strings * switch to insertAdjanceHTML & remove test.sh run is clean.
Revises "globalSP to HTML" aka generating the list of globabl states and properties. * replaces loop of string concatenation with map and join * switch to template strings (a bit messy but see TODO comment) * switch to insertAdjanceHTML & remove * adjust roletype below (ahead of cleanup there) test.sh run is clean.
Revises how global states&properties are added to role=roletype. * remove superfluous if checks * switch to template strings (a bit messy but see TODO comment) * switch to insertAdjanceHTML & remove test.sh run is clean.
Moves cloneWithoutIds() to global scope; prepares for moving loop functions to global scope. test.sh run is clean.
Moves pdef/sdef loop function to global scope; prepares more refactoring. test.sh run is clean.
Moves it to a function in the global scope, part of splitting out chunks. test.sh run is clean.
Moves it to a function in the global scope; part of splitting out chunks. test.sh run is clean.
Moves it to a function in the global scope; part of splitting out chunks. Also repares separating sdef/pdef HTML creation. test.sh run is clean.
Moves it to a function in the global scope; part of splitting out chunks. Needs to run before container-rewrite so that it can reuse the querySelectorAll. test.sh run is clean.
Splits the temporary handleStatesAndProperties() into populatePropList() and populateGlobalSP(). Note. Slightly changes globalSP by re-using all of the propList entry, more specifically the (at this point still empty) array in the role property of each propList entry. test.sh run is clean.
Extracts the container rewrite and re-uses the (now renamed) helper from sdef/pdef loop. test.sh run is clean.
Sets id and role for the description in HTML creation instead of propList creation. At least setting the ID makes sense here. test.sh run is clean.
|
Ugh. The last change (meant to fix roleInfo output) has regressions in the HTML output. |
Fixes the HTML output. test.sh run is clean.
Adds createDescendantRoles() which encapsulates the getAllSubRoles() to allow for extraction of propList loop. test.sh run is clean.
Extracts the loop into a function in the global scope; not pretty but hopefully better than before. test.sh run is clean.
Moves it into the global scope. test.sh run is clean.
Moves child role HTML generation into generateHTMLRoleChildren() in global scope. test.sh run is clean.
|
I found a slightly hacky way to generate roleInfo.js using a "light" dom implementation in NodeJS, i.e., just using the raw index.html and aria.js. It could be made non-hacky if we decide to switch to ESM - but that would require small changes in the spec to allow for asynchronous code. |
Changes subRoles to a simple object of sets (instead of an overloaded array). * createDescendantRoles(): populate subRoles with sets * createDescendantRoles(): adjust to new subRoles structure; also adds TODO * generateHTMLRoleChildren() * change signature to work in Object.entries loop * adjust to new subRoles structure * ariaAttributeReferences() * change subRoles to object * adjust loop to iterate over entries. test.sh run is clean.
Instead of declaring subRoles and populating it, generateSubRoles takes rdefs and returns subRoles. * generateSubRoles(): creates subRoles object and loops over given rdefs to populate it * ariaAttributeReferences(): replace populateSubRoles() with generateSubRoles() test.sh run is clean.
|
One less overloaded array construction. |
… sets * createDescendantRoles(): rewrite getAllSubRoles() to produce sets as values in descendantRoles * propListLoop(): adjust to new descendantRoles structure test.sh run is clean.
…edRoles) Hopefully simplifies this. But also a big TODO question. test.sh run is clean.
Remove some unnecessary spread operations. test.sh run is clean.
Simplify sortedList and add TODO / note on bug. test.sh run is clean.
Get rid of placeholder variable by moving the querySelector to its only use. Note: if the placeholder doesn't exist, we should fix something. test.sh run is clean.
Get rid of the prev variable; hopefully makes it clearer what we're doing. Also adds more TODOs for additional clarifications in the future. test.sh run is clean. roleInfo exports identically.
Replaces super-todo with several questions. Also adds a TODO to the main function just before buildInheritedStatesProperties() test.sh run is clean. roleInfo export is clean.
|
@jnurthen, @spectranaut, I think this is as good as it gets for now. There are plenty of TODOs items noted throughout. I especially failed to really clarify (what is now called) If you want me to change anything to make it easier to review, I'd be happy to. |
|
Hey @pkra, I'm starting to look through all this code -- I never looked at the previous script, so this is really my first introduction, as I said :) I appreciate your test script - it looks like there is no change in the resulting html before and after this refactor, when I run it! I'll take a second closer look soon, but for this PR, do you have any specific goals for the review? Are you curious to just make sure it's readable, the javascript looks sound enough, at this stage? Is your goal to land this pretty close to as-is, so that the follow up work happens in sequential PRs? |
I'd say my primary goals is to establish a fresh baseline we can work on together.
As I said on the last call, I didn't feel knowledgeable enough to go the route of rewriting aria.js from scratch (in James words: because "we know what we want it to do" - even now I wouldn't say the same for myself). I think re-writing it again is a good idea in the long run - I have a lot of stupid ideas for that now - but should probably involve changing the spec markup more heavily, e.g., James' idea of moving closer to "standard" respec markup. Long story short, I'd be fine with landing this as is and doing more work later (which would give the benefit of a full rollout). But I'm also fine continuing work here. |
| applicabilityText === | ||
| "All elements of the base markup except for some roles or elements that prohibit its use"; | ||
| const isDeprecated = | ||
| applicabilityText === "Use as a global deprecated in ARIA 1.2"; |
There was a problem hiding this comment.
I see, this is awful. Definitely, it makes more sense to start with a JSON object rather than looking for this text..
| // if we are in a div, convert that div to a section | ||
| // TODO: | ||
| // a) seems to be always the case. | ||
| // b) Why don't we author the spec this way? |
There was a problem hiding this comment.
Seems like we should. Anything that is not generated should just be in the static html. It's odd because this container has id=<attributename> on it for the url fragment, which probably should be generated, since it is the same as the pdef/sdef
| const itemEntry = propList[title]; | ||
| const dRef = item.nextElementSibling; | ||
| dRef.id = "desc-" + title; // TODO: too much of a side-effect? | ||
| dRef.setAttribute("role", "definition"); // TODO: ditto? |
There was a problem hiding this comment.
this kind of stuff is a little weird to be adding here, it's not generated, it should just be in the html, it seems.
| * | ||
| * @param {HTMLElement} item - rdef element | ||
| */ | ||
| const rewriteRdef = function (item) { |
There was a problem hiding this comment.
at least if this is functionally similar to generateHTMLStatesAndProperties, it should have a similar name?
|
|
||
| pdefsAndsdefs.forEach(populatePropList.bind(null, propList)); | ||
| pdefsAndsdefs.forEach(populateGlobalSP.bind(null, propList, globalSP)); | ||
| pdefsAndsdefs.forEach(generateHTMLStatesAndProperties.bind(null, propList)); |
There was a problem hiding this comment.
Each of these new function names could maybe follow a bit better of conventions... my top preference is at at least to rename any function that changes or produces html to have the prefix render..., and to describe specifically what it is creating -- like, renderStatesAndPropertiesHeadings instead of generateHTMLStatesAndProperties.
Also, related, I guess the "populate" is fine, but I might use a word like build... for anything that is just producing a data structure. Like buildPropList and buildGlobalStatesAndPropertiesList and wherever else data is just being mined from the html and then rearranged.
Also.... I know you are reusing variable names, but personally I would use no short hands -- so, "buildStateAndPropertyList" instead of "buildPropList". I guess "pdefs" and "sdefs" etc are ok, considering they are using in our spec.
but right now I still don't really understand why there is a globalSP and propList and the difference between these two things.. could they somehow be names a bit more clearly? Or are you trying to reuse these original names to make it easier to compare to the original script?
|
Thanks so much, @spectranaut! I'll need a little bit of time to get back into this. |
Cf. #104
Much work to be done.
test.shruns respec on ARIA before/after applying aria.js changes and diffs the two results