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 6 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
16 changes: 11 additions & 5 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 @@ -29,15 +31,19 @@ export default function linkInTextBlockStyleEvaluate(node) {
if (elementIsDistinct(node, parentBlock)) {
return true;
}
if (hasPseudoContent(node)) {
this.data({ messageKey: 'pseudoContent' });
return undefined;

for (const inlineNode of inlineNodes) {
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
85 changes: 53 additions & 32 deletions lib/commons/color/element-is-distinct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Color from './color';
import { getComposedParent } from '../dom';

/**
* Creates a string array of fonts for given CSSStyleDeclaration object
Expand All @@ -25,60 +26,80 @@ function _getFonts(style) {
* @return {Boolean}
*/
function elementIsDistinct(node, ancestorNode) {
var nodeStyle = window.getComputedStyle(node);
const nodeStyle = window.getComputedStyle(node);

// Check if the link has a background
if (nodeStyle.getPropertyValue('background-image') !== 'none') {
return true;
}

// Check if the link has a border or outline
var hasBorder = ['border-bottom', 'border-top', 'outline'].reduce(
straker marked this conversation as resolved.
Show resolved Hide resolved
(result, edge) => {
var borderClr = new Color();
borderClr.parseString(nodeStyle.getPropertyValue(edge + '-color'));

// Check if a border/outline was specified
return (
result ||
// or if the current border edge / outline
(nodeStyle.getPropertyValue(edge + '-style') !== 'none' &&
parseFloat(nodeStyle.getPropertyValue(edge + '-width')) > 0 &&
borderClr.alpha !== 0)
);
},
false
);
const hasBorder = ['border-bottom', 'border-top', 'outline'].some(edge => {
const borderClr = new Color();
borderClr.parseString(nodeStyle.getPropertyValue(edge + '-color'));

// Check if a border/outline was specified
return (
nodeStyle.getPropertyValue(edge + '-style') !== 'none' &&
parseFloat(nodeStyle.getPropertyValue(edge + '-width')) > 0 &&
borderClr.alpha !== 0
);
});

if (hasBorder) {
return true;
}

var parentStyle = window.getComputedStyle(ancestorNode);
// Compare fonts
if (_getFonts(nodeStyle)[0] !== _getFonts(parentStyle)[0]) {
const ancestorStyle = window.getComputedStyle(ancestorNode);
let curNode = node;
while (curNode !== ancestorNode) {
const curNodeStyle = window.getComputedStyle(curNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the first iteration curNodeStyle === nodeStyle. Lets avoid calling this expensive function twice.


// text decoration styles are not inherited so we need
// to check each node from the target node to the
// ancestor node
const hasStyle = ['text-decoration-line', 'text-decoration-style'].some(
straker marked this conversation as resolved.
Show resolved Hide resolved
cssProp => {
const ancestorValue = ancestorStyle.getPropertyValue(cssProp);
const curNodeValue = curNodeStyle.getPropertyValue(cssProp);

/*
For logic purposes we can treat the target node and all inline ancestors as a single logic point since if any of them define a text-decoration style it will visually apply that style to the target.

A target node is distinct if it defines a text-decoration and either the ancestor does not define one or the target's text-decoration is different than the ancestor. A target that does not define a text-decoration can never be distinct from the ancestor.
*/
return (
curNodeValue !== 'none' &&
(ancestorValue === 'none' || curNodeValue !== ancestorValue)
);
}
);

if (hasStyle) {
return true;
}

curNode = getComposedParent(curNode);
}

// font styles are inherited so we only need to check
// the target node
if (_getFonts(nodeStyle)[0] !== _getFonts(ancestorStyle)[0]) {
straker marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

var hasStyle = [
'text-decoration-line',
'text-decoration-style',
'font-weight',
'font-style',
'font-size'
].reduce((result, cssProp) => {
let hasStyle = ['font-weight', 'font-style', 'font-size'].some(cssProp => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this stuff that inherits should go above the while loop. Also, a comment here would be nice to indicate this isn't in the while loop because it inherits. After not having thought about this for three weeks, it took me a bit to realise why you did it this way.

return (
result ||
nodeStyle.getPropertyValue(cssProp) !==
parentStyle.getPropertyValue(cssProp)
ancestorStyle.getPropertyValue(cssProp)
);
}, false);
});

var tDec = nodeStyle.getPropertyValue('text-decoration');
const tDec = nodeStyle.getPropertyValue('text-decoration');
if (tDec.split(' ').length < 3) {
// old style CSS text decoration
hasStyle =
hasStyle || tDec !== parentStyle.getPropertyValue('text-decoration');
hasStyle || tDec !== ancestorStyle.getPropertyValue('text-decoration');
}

return hasStyle;
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');
});
});
Loading