Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions demo/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ <H1>this is a really big heading tag. 51 chars long even. this is a really big h
<p> - List item</p>
<p>Split breaks:</p>
<p>1. List Item<br>2. List Item</p>
<a href="weird link">Link</a>
</textarea>
</div>
</div>
Expand Down
21 changes: 14 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"axios": "^0.18.0",
"format-message": "^6",
"format-message-generate-id": "^6",
"node-fetch": "^2.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have axios in place. It should be able to handle this same sort of thing without needing an additional library involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sweet. I'll just use that then.

"prevent-default": "^1.0.0",
"react": "^0.14.8 || ^15.0.0",
"react-aria-live": "^1.0.4",
Expand Down
26 changes: 26 additions & 0 deletions src/rules/__tests__/__snapshots__/valid-links.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`data returns the proper object 1`] = `
Object {
"href": null,
}
`;

exports[`form returns the proper object 1`] = `
Array [
Object {
"dataKey": "href",
"disabledIf": [Function],
"label": "Ensure this link is correct.",
},
Object {
"checkbox": true,
"dataKey": "ignore",
"label": "Ignore this link in the future.",
},
]
`;

exports[`message returns the proper message 1`] = `"This link could not be verified."`;

exports[`why returns the proper why message 1`] = `"Links should not be broken."`;
89 changes: 89 additions & 0 deletions src/rules/__tests__/valid-links.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import fetch from "node-fetch"
import rule from "../valid-links"

let body, a

beforeEach(() => {
body = document.createElement("body")
a = document.createElement("a")
body.appendChild(a)
a.textContent = "Link Text"
window.fetch = () => Promise.resolve({ ok: true })
})

describe("test", () => {
test("returns true if not A element", async () => {
expect(await rule.test(document.createElement("div"))).toBe(true)
})

test("returns true for valid links", async () => {
window.fetch = () => Promise.resolve({ ok: true })
a.setAttribute("href", "https://www.instructure.com/")
expect(await rule.test(a)).toBe(true)
})

test("returns false for invalid links", async () => {
window.fetch = () => Promise.resolve({ ok: false })
a.setAttribute("href", "http://something weird")
expect(await rule.test(a)).toBe(false)
a.setAttribute("href", "plaintext")
expect(await rule.test(a)).toBe(false)
window.fetch = () => Promise.reject(new Error())
a.setAttribute("href", "http://something-valid-but-not-reachable/123")
expect(await rule.test(a)).toBe(false)
})
})

describe("data", () => {
test("returns the proper object", () => {
expect(rule.data(a)).toMatchSnapshot()
})
})

describe("form", () => {
test("returns the proper object", () => {
expect(rule.form(a)).toMatchSnapshot()
})
})

describe("update", () => {
test("returns same element", () => {
expect(rule.update(a, {})).toBe(a)
})
test("does not change href if href does not change", () => {
const href = "http://example.com"
a.setAttribute("href", href)
expect(rule.update(a, { href })).toBe(a)
})
test("changes href if it has been changed", () => {
const href = "bad"
a.setAttribute("href", href)
rule.update(a, { href: "https://www.instructure.com/" })
expect(a.getAttribute("href")).toBe("https://www.instructure.com/")
})
test("does not change href if href does not change", () => {
const href = "http://example.com"
a.setAttribute("href", href)
expect(
rule.update(a, { ignore: true }).getAttribute("data-ignore-a11y-check")
).toBe("true")
})
})

describe("rootNode", () => {
test("returns the parentNode of an element", () => {
expect(rule.rootNode(a).tagName).toBe("BODY")
})
})

describe("message", () => {
test("returns the proper message", () => {
expect(rule.message()).toMatchSnapshot()
})
})

describe("why", () => {
test("returns the proper why message", () => {
expect(rule.why()).toMatchSnapshot()
})
})
4 changes: 3 additions & 1 deletion src/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import headingsSequence from "./headings-sequence"
import imageAltLength from "./img-alt-length"
import paragraphsForHeadings from "./paragraphs-for-headings"
import listStructure from "./list-structure"
import validLinks from "./valid-links"

export default [
imgAlt,
Expand All @@ -23,5 +24,6 @@ export default [
headingsSequence,
imageAltLength,
paragraphsForHeadings,
listStructure
listStructure,
validLinks
]
109 changes: 109 additions & 0 deletions src/rules/valid-links.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import formatMessage from "../format-message"

const isValidURL = url => {
try {
// the URL constructor is more accurate than regex
// but not supported in IE.
new URL(url)
return true
} catch (_) {
// If this does throw, either:
// 1. the url is invalid
// 2. the URL constructor is not there.
// The user will be prompted to check the link manually.
return false
}
}

const debouncedFetch = (() => {
let timeout = null

return (...args) =>
new Promise((resolve, reject) => {
clearTimeout(timeout)
timeout = setTimeout(() => {
fetch(...args)
.then(resolve)
.catch(reject)
}, 500)
})
})()

export default {
test: function(elem) {
return new Promise((resolve, reject) => {
if (elem.tagName !== "A") return resolve(true)
const href = elem.getAttribute("href")

// If url is invalid
if (!isValidURL(href)) return resolve(false)

// If url is valid, do a HEAD request.
debouncedFetch(href, { method: "HEAD" })
.then(res => resolve(res.ok))
.catch(() => resolve(false))

// This will always work in jest because there
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense, the limitations with CORS requests from AJAX definitely cause issues here. I'm not sure we want to deal with making a service to handle this or the complexity of introducing service workers to a plugin like this.

I'll see about getting some clarification on how we want to proceed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Synvox I've talked with some stakeholders internally on how this should go... for now.. you can just go with your original "link schema checker" rather than the full on "link validator"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was thinking. It is possible to check same origin links but much more complicated to do cross-origin. There may be a way to reference a foreign resource through an <img> tag or similar, and check to see if the image fails to load because of a failing status code. It's hacky, but similar to how JSONP works.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true. If you wanna do some experimentation with that... that could be really cool. We do something similar to that to detect broken images in canvas-lms by adding a onerror handler to images. Kinda hacky, but maybe workable to do something like that here.(https://github.com/instructure/canvas-lms/blob/master/app/coffeescripts/behaviors/broken-images.js)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a thought. https://codepen.io/Synvox/pen/pZvYPb?editors=0012

It looks like IMG tags don't report their failure reason which makes it impossible to detect if it failed because of a 404, or if it failed because it tried to load a document instead of an image.

The alternative is to load the resource in a Worker. The worker only throws when the resource fails on the network, which is exactly what we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at that... it seems like a good solution... they aren't supported in IE 11, but I think we could probably just let that be a restriction on the plugin. If you aren't in IE then you get a little bit more validation, otherwise you get the basic stuff.

// node does not check cross-origin requests,
// but this will cause issues in the browser.
//
// In the browser, the request will still fire
// but will will fail for all links that are
// not same-origin.
//
// For cross-origin urls, the user will be
// prompted to check the link, or check ignore
// if they so choose.
//
// Workarounds:
// * Create a service to proxy the request
// * Do something crazy with web workers
// * and importScripts (plausible).
})
},

data: elem => {
return {
href: elem.getAttribute("href")
}
},

form: () => [
{
label: formatMessage("Ensure this link is correct."),
dataKey: "href",
disabledIf: data => data.ignore
},
{
label: formatMessage("Ignore this link in the future."),
checkbox: true,
dataKey: "ignore"
}
],

update: function(elem, data) {
const rootElem = elem.parentNode

if (data.ignore) {
elem.setAttribute("data-ignore-a11y-check", "true")
}

if (data.href !== elem.getAttribute("href")) {
elem.setAttribute("href", data.href)
}

return elem
},

rootNode: function(elem) {
return elem.parentNode
},

// Note, these messages are poor and should be replaced with
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll get some words for you here :)

// better text.
message: () => formatMessage("This link could not be verified."),

why: () => formatMessage("Links should not be broken."),

link: " --- fill in --- "
}