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.

Sign up for GitHub

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(link-in-text-block): take into account ancestor inline element styles #4135

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

fix(link-in-text-block): take into account ancestor inline element styles #4135

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bc4bf09
fix(link-in-text-block): take into account ancestor inline element st…
straker Aug 24, 2023
47bf2b5
integration tests
straker Aug 24, 2023
859ffcf
typo
straker Aug 24, 2023
c50700e
fix same color issue
straker Aug 24, 2023
470b462
add u example
straker Aug 24, 2023
2f100c1
figure out text-decoration
straker Aug 30, 2023
014585f
fix tests
straker Aug 31, 2023
5ebde67
Merge branch 'develop' into link-in-text-style-ancestor
straker Aug 31, 2023
4d839b9
suggestions
straker Sep 20, 2023
25b3001
fix test
straker Sep 20, 2023
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
22 changes: 14 additions & 8 deletions lib/checks/color/link-in-text-block-style-evaluate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ export default function linkInTextBlockStyleEvaluate(node) {
return false;
}

var parentBlock = getComposedParent(node);
let parentBlock = getComposedParent(node);
const inlineNodes = [node];
while (parentBlock && parentBlock.nodeType === 1 && !isBlock(parentBlock)) {
inlineNodes.push(parentBlock);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can just push any inline ancestor. If the element has non-link text in it is no longer a distinguishing style. Take something like the following, that should not pass because it has a bold parent:

<p>
   Welcome to the jungle <strong>we have <a href="">fun and games</a></strong>.
</p>

This probably does need to do some filtering. It doesn't matter if it's just a comma or something, but whole worlds I don't think counts anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

The above issue was resolved for strong / underline, but not for pseudo elements. This shouldn't be passed because of that pseudoElm class:

<p>
   Welcome to the jungle <span class="pseudoElm">we have <a href="">fun and games</a></span>.
</p>

It makes me wonder whether checking for pseudo elms should be done inside elementIsDistinct to avoid the repeat?

straker marked this conversation as resolved.
Show resolved Hide resolved
parentBlock = getComposedParent(parentBlock);
}

Expand All @@ -26,18 +28,22 @@ export default function linkInTextBlockStyleEvaluate(node) {

this.relatedNodes([parentBlock]);

if (elementIsDistinct(node, parentBlock)) {
return true;
}
if (hasPseudoContent(node)) {
this.data({ messageKey: 'pseudoContent' });
return undefined;
for (const inlineNode of inlineNodes) {
if (elementIsDistinct(inlineNode, parentBlock)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

if (hasPseudoContent(inlineNode)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
this.data({ messageKey: 'pseudoContent' });
return undefined;
}
}

return false;
}

function isBlock(elm) {
var display = window.getComputedStyle(elm).getPropertyValue('display');
const display = window.getComputedStyle(elm).getPropertyValue('display');
return blockLike.indexOf(display) !== -1 || display.substr(0, 6) === 'table-';
}

Expand Down
138 changes: 83 additions & 55 deletions test/checks/color/link-in-text-block-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,24 +71,71 @@ describe('link-in-text-block-style', () => {
styleElm.innerHTML += selector + ' {\n' + cssLines + '\n}\n';
}

function getLinkElm(linkStyle) {
function getLinkElm(linkStyle, spanStyle = {}) {
// Get a random id and build the style strings
const linkId = 'linkid-' + Math.floor(Math.random() * 100000);
const parId = 'parid-' + Math.floor(Math.random() * 100000);
const spanId = 'spanid-' + Math.floor(Math.random() * 100000);

createStyleString('#' + linkId, linkStyle);
createStyleString('#' + parId, {});
createStyleString('#' + spanId, spanStyle);

fixture.innerHTML += `
<p id="${parId}">
<span id="${spanId}">
<a href="/" id="${linkId}">link</a>
</span>
</p>
`;

fixture.innerHTML +=
'<p id="' +
parId +
'"> Text ' +
'<a href="/" id="' +
linkId +
'">link</a>' +
'</p>';
axe.testUtils.flatTreeSetup(fixture);
return document.getElementById(linkId);
return {
parentElm: document.getElementById(parId),
linkElm: document.getElementById(linkId)
};
}

function createPseudoTests(elmName, desc) {
it(`returns undefined if ${desc} has a :before pseudo element`, () => {
const link = queryFixture(`
<style>
${elmName}:before { content: '🔗'; }
a { text-decoration: none; }
</style>
<p>A <span><a href="#" id="target">link</a></span> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isUndefined(result);
assert.deepEqual(checkContext._data, { messageKey: 'pseudoContent' });
assert.equal(checkContext._relatedNodes[0], link.parentNode.parentNode);
});

it(`returns undefined if ${desc} has a :after pseudo element`, () => {
const link = queryFixture(`
<style>
${elmName}:after { content: ""; }
a { text-decoration: none; }
</style>
<p>A <span><a href="#" id="target">link</a></span> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isUndefined(result);
assert.deepEqual(checkContext._data, { messageKey: 'pseudoContent' });
assert.equal(checkContext._relatedNodes[0], link.parentNode.parentNode);
});

it(`does not return undefined if ${desc} pseudo content is none`, () => {
const link = queryFixture(`
<style>
${elmName}:after { content: none; position: absolute; }
a { text-decoration: none; }
</style>
<p>A <span><a href="#" id="target">link</a></span> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isFalse(result);
});
}

describe('link default state', () => {
Expand Down Expand Up @@ -148,60 +195,41 @@ describe('link-in-text-block-style', () => {
});

describe('links distinguished through style', () => {
const testSuites = {
underline: { textDecoration: 'underline' },
border: { 'border-bottom': '1px solid' },
outline: { outline: '1px solid' },
'font-weight': { 'font-weight': 'bold' },
'font-size': { 'font-size': '2rem' },
'background-image': { 'background-image': 'url()' }
};

it('returns false if link style matches parent', () => {
const linkElm = getLinkElm({});
const { linkElm, parentElm } = getLinkElm({});
assert.isFalse(linkInBlockStyleCheck.call(checkContext, linkElm));
assert.equal(checkContext._relatedNodes[0], linkElm.parentNode);
assert.equal(checkContext._relatedNodes[0], parentElm);
assert.isNull(checkContext._data);
});

it('returns true if link has underline', () => {
const linkElm = getLinkElm({
textDecoration: 'underline'
Object.entries(testSuites).forEach(([property, styles]) => {
it(`returns true if link has ${property}`, () => {
straker marked this conversation as resolved.
Show resolved Hide resolved
const { linkElm, parentElm } = getLinkElm(styles);
assert.isTrue(linkInBlockStyleCheck.call(checkContext, linkElm));
assert.equal(checkContext._relatedNodes[0], parentElm);
assert.isNull(checkContext._data);
});
assert.isTrue(linkInBlockStyleCheck.call(checkContext, linkElm));
assert.equal(checkContext._relatedNodes[0], linkElm.parentNode);
assert.isNull(checkContext._data);
});

it('returns undefined when the link has a :before pseudo element', () => {
const link = queryFixture(`
<style>
a:before { content: '🔗'; }
a { text-decoration: none; }
</style>
<p>A <a href="#" id="target">link</a> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isUndefined(result);
assert.deepEqual(checkContext._data, { messageKey: 'pseudoContent' });
assert.equal(checkContext._relatedNodes[0], link.parentNode);
});

it('returns undefined when the link has a :after pseudo element', () => {
const link = queryFixture(`
<style>
a:after { content: ""; }
a { text-decoration: none; }
</style>
<p>A <a href="#" id="target">link</a> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isUndefined(result);
assert.deepEqual(checkContext._data, { messageKey: 'pseudoContent' });
assert.equal(checkContext._relatedNodes[0], link.parentNode);
Object.entries(testSuites).forEach(([property, styles]) => {
it(`returns true if ancestor inline element has ${property}`, () => {
const { linkElm, parentElm } = getLinkElm({}, styles);
assert.isTrue(linkInBlockStyleCheck.call(checkContext, linkElm));
assert.equal(checkContext._relatedNodes[0], parentElm);
assert.isNull(checkContext._data);
});
});

it('does not return undefined when the pseudo element content is none', () => {
const link = queryFixture(`
<style>
a:after { content: none; position: absolute; }
a { text-decoration: none; }
</style>
<p>A <a href="#" id="target">link</a> inside a block of text</p>
`).actualNode;
const result = linkInBlockStyleCheck.call(checkContext, link);
assert.isFalse(result);
});
createPseudoTests('a', 'link');
createPseudoTests('span', 'ancestor inline element');
});
});
30 changes: 28 additions & 2 deletions test/integration/rules/link-in-text-block/link-in-text-block.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ <h1>Non-color change tests</h1>
<p style="color: black">
paragraph of text (pass: link is bolder than paragraph)
<a
style="text-decoration: none; font-weight: bolder; color: black"
style="text-decoration: none; font-weight: bolder; color: #222"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would pass even without the change in style due to using the same color as the paragraph.

href="#"
id="pass-different-weight"
>
Expand All @@ -66,14 +66,40 @@ <h1>Non-color change tests</h1>
<p style="color: black">
paragraph of text (pass: link is larger that paragraph)
<a
style="text-decoration: none; font-size: larger; color: black"
style="text-decoration: none; font-size: larger; color: #222"
href="#"
id="pass-different-size"
>
Link text</a
>
</p>

<p style="color: black">
paragraph of text (pass: ancestor span is bolder than paragraph)
<span style="font-weight: bolder">
<a
style="text-decoration: none; color: #222"
href="#"
id="pass-ancestor-different-weight"
>
Link text</a
>
</span>
</p>

<p style="color: black">
paragraph of text (pass: ancestor span uses border)
<span style="border-bottom: 1px solid">
<a
style="text-decoration: none; color: #222"
href="#"
id="pass-ancestor-border"
>
Link text</a
>
</span>
</p>

<h1>Color change tests</h1>
<b
>Note that these tests limit changes to one variable per test. No attempt is
Expand Down
2 changes: 2 additions & 0 deletions test/integration/rules/link-in-text-block/link-in-text-block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
["#pass-default-styling-aria-hidden"],
["#pass-different-weight"],
["#pass-different-size"],
["#pass-ancestor-different-weight"],
["#pass-ancestor-border"],
["#pass-background-color"],
["#pass-text-color"],
["#pass-same-colors"]
Expand Down