editorial: refactor aria.js #2408

pkra · 2025-01-03T16:12:25Z

Part of #2230

Partially moved from w3c/aria-common#106 at caf8e9e

From the original PR:

Much work to be done.

add a very simple test
- test.sh runs respec on ARIA before/after applying aria.js changes and diffs the two results
rough goals
- clarify (e.g., switch to const/let, replace reused variable names with unique ones)
- simplify (e.g., loops over querySelectorAll, use sets instead of filtering)
- revise HTML string creation
- extract loops
steps
- pdef, sdef loop
- generating HTML for indices of (global) states&properties as well as role=roletype
- rdef loop
- roleInfo loop
- propList loop
- generate HTML for role-related indices, sub-Roles
- clean-up
continuation
- rewrite subRoles code

Preview | Diff

netlify · 2025-01-03T16:12:55Z

✅ Deploy Preview for wai-aria ready!

Name	Link
🔨 Latest commit	`555a28a`
🔍 Latest deploy log	https://app.netlify.com/sites/wai-aria/deploys/67f4d4405c2de30008320d90
😎 Deploy Preview	https://deploy-preview-2408--wai-aria.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copies the proposed refactor from w3c/aria-common#106 at caf8e9e The PR has many separate commits for easier review.

Copies from w3c/aria-common#106 at caf8e9e The PR has many separate commits for easier review.

pkra · 2025-03-21T14:38:47Z

common/script/aria.js

+    // 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?


Copied from w3c/aria-common#106 (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

pkra · 2025-03-21T14:39:13Z

common/script/aria.js

+    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?


From w3c/aria-common#106 (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.

pkra · 2025-03-21T14:39:52Z

common/script/aria.js

+
+    pdefsAndsdefs.forEach(populatePropList.bind(null, propList));
+    pdefsAndsdefs.forEach(populateGlobalSP.bind(null, propList, globalSP));
+    pdefsAndsdefs.forEach(generateHTMLStatesAndProperties.bind(null, propList));


From w3c/aria-common#106 (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?

pkra · 2025-03-21T14:40:23Z

common/script/aria.js

+        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";


From w3c/aria-common#106 (comment)

I see, this is awful. Definitely, it makes more sense to start with a JSON object rather than looking for this text..

pkra · 2025-03-21T14:42:18Z

After the individual comments above, here's the closing comment from @spectranaut's review at w3c/aria-common#106 (review)

Ok Peter I took another crack at going over this -- I wonder how much you would be interested in renaming functions or variables in this PR? :)

pkra · 2025-03-21T14:42:52Z

I wonder how much you would be interested in renaming functions or variables in this PR? :)

I'm interested and I will try to get to this before the editors' call next week. (fingers crossed)

Following reviewer comments - render***: (primarily) generates HTML - build***: (primarily) extracts data for rendering & roleInfo.js - a few comments added or tweaked Respec output with only expected changes.

spectranaut · 2025-03-26T15:37:43Z

whoo!! I'll take a look tomorrow!

spectranaut · 2025-03-27T23:08:02Z

Ok @pkra -- what do you think should be the goal here? How close of a review do you want at this stage?

Should we just merge this, as an intermediate step, then decide how to clean up more in the future? I'm fine with merging it as is without a close review, as it's just clean up and the spec is the same.

pkra · 2025-03-28T08:50:00Z

Should we just merge this, as an intermediate step, then decide how to clean up more in the future?

That would be my suggestion. That way, we can start fixing the bugs we already know about (cf. the editors' board's "blocked" column). And of course we should continue to refactor it (incl. changing the spec to simplify the post-processing).

I'm fine with merging it as is without a close review, as it's just clean up and the spec is the same.

I did my best to test that, diffing local respec builds before/after. The local respec builds would always fail to find the funders.html include so I checked against the gh-pages branch just now, too.

pkra · 2025-03-28T08:57:30Z

I've moved the TODOs from the top card to #2501

pkra requested a review from spectranaut January 3, 2025 16:12

pkra added Agenda-Editors editorial a change to an example, note, spelling, grammar, or is related to publishing or the repo labels Jan 3, 2025

pkra added this to the 2025 milestone Jan 3, 2025

pkra mentioned this pull request Jan 3, 2025

refactor(aria.js): first steps w3c/aria-common#106

Closed

12 tasks

pkra mentioned this pull request Feb 21, 2025

[aria] Request: Provide the ARIA “Characteristics” data as JSON or in some other machine-readable form #2445

Open

pkra removed the Agenda-Editors label Mar 4, 2025

spectranaut mentioned this pull request Mar 6, 2025

Review deprecation processes for ARIA attributes (and roles?) #1863

Open

pkra added 2 commits March 17, 2025 12:29

editorial: refactor aria.js

bf14cf0

Copies the proposed refactor from w3c/aria-common#106 at caf8e9e The PR has many separate commits for easier review.

editorial: add simple test script for aria.js

d167788

Copies from w3c/aria-common#106 at caf8e9e The PR has many separate commits for easier review.

pkra force-pushed the ariaCommonIssue104 branch from 717ecc5 to d167788 Compare March 17, 2025 11:46

chore: prettier

f51f921

pkra mentioned this pull request Mar 20, 2025

editorial: follow ups to removal of common.css #2462

Closed

4 tasks

pkra commented Mar 21, 2025

View reviewed changes

pkra and others added 3 commits March 26, 2025 09:42

Merge branch 'main' into ariaCommonIssue104

cce25ed

chore: prettier

fbc3399

editorial: rename functions in aria.js

d2bbc1e

Following reviewer comments - render***: (primarily) generates HTML - build***: (primarily) extracts data for rendering & roleInfo.js - a few comments added or tweaked Respec output with only expected changes.

pkra marked this pull request as ready for review March 26, 2025 10:37

github-actions bot added the spec:aria label Mar 26, 2025

pkra mentioned this pull request Mar 28, 2025

continue aria.js work #2501

Open

15 tasks

Merge branch 'main' into ariaCommonIssue104

72a5c49

github-actions bot removed the spec:aria label Mar 28, 2025

Merge branch 'main' into ariaCommonIssue104

555a28a

spectranaut approved these changes Apr 8, 2025

View reviewed changes

pkra merged commit b085915 into main May 8, 2025
7 checks passed

github-project-automation bot moved this from In Progress to Done in ARIA Editors May 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

editorial: refactor aria.js #2408

editorial: refactor aria.js #2408

Uh oh!

pkra commented Jan 3, 2025 •

edited by pr-preview bot

Loading

Uh oh!

netlify bot commented Jan 3, 2025 •

edited

Loading

Uh oh!

pkra Mar 21, 2025

Uh oh!

pkra Mar 21, 2025

Uh oh!

pkra Mar 21, 2025

Uh oh!

pkra Mar 21, 2025

Uh oh!

pkra commented Mar 21, 2025

Uh oh!

pkra commented Mar 21, 2025

Uh oh!

spectranaut commented Mar 26, 2025

Uh oh!

spectranaut commented Mar 27, 2025

Uh oh!

pkra commented Mar 28, 2025

Uh oh!

pkra commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

editorial: refactor aria.js #2408

editorial: refactor aria.js #2408

Uh oh!

Conversation

pkra commented Jan 3, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for wai-aria ready!

Uh oh!

pkra Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

pkra Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

pkra Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

pkra Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

pkra commented Mar 21, 2025

Uh oh!

pkra commented Mar 21, 2025

Uh oh!

spectranaut commented Mar 26, 2025

Uh oh!

spectranaut commented Mar 27, 2025

Uh oh!

pkra commented Mar 28, 2025

Uh oh!

pkra commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

pkra commented Jan 3, 2025 •

edited by pr-preview bot

Loading

netlify bot commented Jan 3, 2025 •

edited

Loading