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

Fix references to not be hardcoded #1240

Merged
merged 12 commits into from
Apr 22, 2020
Merged

Fix references to not be hardcoded #1240

merged 12 commits into from
Apr 22, 2020

Conversation

jnurthen
Copy link
Member

@jnurthen jnurthen commented Apr 16, 2020

closes #1143


Preview | Diff

@jnurthen
Copy link
Member Author

related issue #1214 but this PR is not attempting to resolve everything in that one.

@jnurthen
Copy link
Member Author

@carmacleod The reference to text nodes is to resolve an issue where term references are not being resolved correctly when a term references another term.
In short - the easiest way to fix it was to find a reference to "text node" and "node" in the document and add them.
I should probably just do this in a separate PR though.

@carmacleod
Copy link
Contributor

@jnurthen My point was that it doesn't need a separate PR because that line is completely deleted in #1100.

@jnurthen
Copy link
Member Author

I'll have to find another place to reference it then... or fix the script.

@jnurthen jnurthen requested review from joanmarie and mcking65 April 16, 2020 17:51
Copy link
Contributor

@joanmarie joanmarie left a comment

Choose a reason for hiding this comment

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

Looking at the preview, something seems to be causing Core-AAM 1.1 to still be pulled in. https://pr-preview.s3.amazonaws.com/w3c/aria/pull/1240.html#informative-references

That said, all the changes made here need making. So +1 from me.

@carmacleod
Copy link
Contributor

Good catch, @joanmarie.

@jnurthen Can we strip off the # whenever href="#" so that, for example, instead of https://w3c.github.io/aria-practices/# we would have https://w3c.github.io/aria-practices/ ?

@jnurthen
Copy link
Member Author

Good catch, @joanmarie.

@jnurthen Can we strip off the # whenever href="#" so that, for example, instead of https://w3c.github.io/aria-practices/# we would have https://w3c.github.io/aria-practices/ ?

Tell me how and I'll do it.

@carmacleod
Copy link
Contributor

carmacleod commented Apr 16, 2020

I believe it's line 42 of resolveReferences.js, but I wouldn't swear to it... :)

Maybe add if (href === '#') href = ''; right before line 42?

So you have:

        var href = $ (el).attr ('href');
        if (href === '#') href = '';
        $ (el).attr ('href', baseUrl + href);

@jnurthen
Copy link
Member Author

Actually - it seems like changing href="#" to href="" is good enough.

@carmacleod
Copy link
Contributor

Heh - that's one way to do it. :)

@carmacleod
Copy link
Contributor

carmacleod commented Apr 17, 2020

Should [CORE-AAM-1.2] and [ACCNAME-1.2] be listed under normative references? They are in the informative references list in this PR Preview, because the only instances are in the informative intro.

@jnurthen
Copy link
Member Author

Should [CORE-AAM-1.2] and [ACCNAME-1.2] be listed under normative references? They are in the informative references list in this PR Preview, because the only instances are in the informative intro.

Yes. I will change at least one of the others to reference.

jnurthen and others added 4 commits April 16, 2020 20:18
Co-Authored-By: Carolyn MacLeod <[email protected]>
Co-Authored-By: Carolyn MacLeod <[email protected]>
Co-Authored-By: Carolyn MacLeod <[email protected]>
jnurthen and others added 4 commits April 16, 2020 20:21
@carmacleod
Copy link
Contributor

Thanks for the updates! Regarding the strange informative [CORE-AAM-1.1] reference, it might (?) be coming from an alias in localBiblio.js in aria-common.

Regardless, there's a few aliases in there that should be updated in partnership with this PR:

"ACCNAME-AAM": {
	"aliasOf": "ACCNAME-AAM-1.1",
},
"ARIA-PRACTICES": {
	"aliasOf": "WAI-ARIA-PRACTICES-1.1",
},
"CORE-AAM": {
	"aliasOf": "CORE-AAM-1.1",
},

I think they need to change to the following (keeping ACCNAME-AAM in case other aria-common docs are using it):

"ACCNAME-AAM": {
	"aliasOf": "ACCNAME-1.2",
},
"ACCNAME": {
	"aliasOf": "ACCNAME-1.2",
},
"ARIA-PRACTICES": {
	"aliasOf": "WAI-ARIA-PRACTICES-1.2",
},
"CORE-AAM": {
	"aliasOf": "CORE-AAM-1.2",
},

@jnurthen
Copy link
Member Author

My guess as to where it is coming from:

  • terms.html in aria-common

Even though it doesn't appear in any of the terms which are defined in our document I'm betting there is a timing thing where the references are getting resolved before terms which are not relevant are getting deleted from the document. This would explain why you don't get this reference when running from the local file system as the terms are not resolved in this case.

@carmacleod
Copy link
Contributor

https://github.com/w3c/aria-common/blob/master/terms.html#L78

    <dt><dfn>Expose</dfn></dt>
    <dd>
      <p>Translated to platform-specific <a class="termref" data-lt="accessibility api">accessibility APIs</a> as defined in the <a href="" class="core-mapping">Core Accessibility API Mappings</a>. [[CORE-AAM-1.1]]</p>
    </dd>

So, I happen to be making changes to terms.html in aria-common at this very moment (updating a bunch of links in the definition of Accessibility API). I can tack that on to my changes if you like.

@carmacleod
Copy link
Contributor

carmacleod commented Apr 17, 2020

Done. CORE-AAM-1.1 reference updated in terms.html in aria-common in w3c/aria-common#42.
(Turns out, there were a couple of other 1.1's in terms.html that I also updated to 1.2's in that PR).
Please review.

Copy link
Member

@michael-n-cooper michael-n-cooper left a comment

Choose a reason for hiding this comment

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

My review wasn't requested but I saw it go by in context of another. :) It looks good. Changes I'm making to how cross references are maintained in the suite should be implemented after this pull request is approved.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

+1
All issues addressed.
Looks good!

@jnurthen jnurthen merged commit 09d27e5 into master Apr 22, 2020
jnurthen added a commit that referenced this pull request Apr 22, 2020
carmacleod added a commit that referenced this pull request May 7, 2020
@jnurthen jnurthen deleted the referenceFix branch July 23, 2020 22:19
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.

1.2 ARIA references 1.1 APG
4 participants