Open pkra opened 9 months ago
Another "fun" find: pdef/sdef and rdef are written to unnecessarily different HTML - p/sdef have an extra nested span inside the h4.
Another "fun" find: pdef/sdef and rdef are written to unnecessarily different HTML - p/sdef have an extra nested span inside the h4.
I guess fixing #105 would add another complication but it feels still the right thing to create similar looking HTML in the long run.
Huh.
https://github.com/w3c/aria-common/blob/2fe4fe20cffad6314bddb6d6b9ed09c63c087cfb/script/aria.js#L216
and then for each rdef we push and item but also add a property with an array.
Sounds like we want an object here.
Sounds like we want an object here.
But of course this once again breaks things in mysterious ways.
We'll need to change eslint config to set ecmaVersion to latest (so we can use optional chaining).
rdef loop refactoring seems complete (for now).
The roleInfo loop is somewhat complete. The overloaded array pattern needs resolving eventually.
Ugh. The last change (meant to fix roleInfo output) has regressions in the HTML output.
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.
One less overloaded array construction.
@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) buildInheritedStatesProperties()
. As noted in comments, I think I've identified a few bugs (both specifically to buildInheritedStatesProperties
and more generally).
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?
do you have any specific goals for the review?
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.
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.sh
runs respec on ARIA before/after applying aria.js changes and diffs the two results