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

Jump to bottom

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 all 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
142 changes: 85 additions & 57 deletions lib/commons/color/element-is-distinct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,6 @@
import Color from './color';

/**
* Creates a string array of fonts for given CSSStyleDeclaration object
* @private
* @param {Object} style CSSStyleDeclaration object
* @return {Array}
*/
function _getFonts(style) {
return style
.getPropertyValue('font-family')
.split(/[,;]/g)
.map(font => {
return font.trim().toLowerCase();
});
}
import { sanitize } from '../text';
import { getComposedParent } from '../dom';

/**
* Determine if the text content of two nodes is styled in a way that they can be distinguished without relying on color
Expand All @@ -24,64 +11,105 @@ function _getFonts(style) {
* @param {HTMLElement} ancestorNode The ancestor node element to check
* @return {Boolean}
*/
function elementIsDistinct(node, ancestorNode) {
var nodeStyle = window.getComputedStyle(node);
export default function elementIsDistinct(node, ancestorNode) {
const nodeStyle = window.getComputedStyle(node);

// Check if the link has a background
if (nodeStyle.getPropertyValue('background-image') !== 'none') {
return true;
}
const ancestorStyle = window.getComputedStyle(ancestorNode);
let curNode = node;
while (curNode !== ancestorNode) {
straker marked this conversation as resolved.
Show resolved Hide resolved
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.


// 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
);

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

if (
hasSameText(node, curNode) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused, why is hasSameText() not in the while condition? I.e.

while (curNode !== ancestorNode && hasSameText(node, curNode)) {

My intuition says that that's what should be happening. Remind me why an ancestor with a background-image that has different text would count as making the link distinct?

(hasVisibleBorder(curNodeStyle) ||
hasVisibleTextDecortation(curNodeStyle, ancestorStyle))
) {
return true;
}

curNode = getComposedParent(curNode);
}

var parentStyle = window.getComputedStyle(ancestorNode);
// Compare fonts
if (_getFonts(nodeStyle)[0] !== _getFonts(parentStyle)[0]) {
// font styles are inherited so we only need to check
// the target node
if (getFonts(nodeStyle)[0] !== getFonts(ancestorStyle)[0]) {
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;
}

export default elementIsDistinct;
/**
* Creates a string array of fonts for given CSSStyleDeclaration object
* @private
* @param {Object} style CSSStyleDeclaration object
* @return {Array}
*/
function getFonts(style) {
return style
.getPropertyValue('font-family')
.split(/[,;]/g)
.map(font => {
return font.trim().toLowerCase();
});
}

function hasSameText(node, ancestorNode) {
// 3 is an arbitrary number that helps account for punctuation
// and emoji that takes up two characters
return ancestorNode.textContent
.split(node.textContent)
.every(text => sanitize(text).length <= 3);
}

function hasVisibleBorder(nodeStyle) {
// Check if the link has a border or outline
return ['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
);
});
}

function hasVisibleTextDecortation(nodeStyle, ancestorStyle) {
// Check if the link has text-decoration styles
return ['text-decoration-line', 'text-decoration-style'].some(cssProp => {
const ancestorValue = ancestorStyle.getPropertyValue(cssProp);
const nodeValue = nodeStyle.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 (
nodeValue !== 'none' &&
(ancestorValue === 'none' || nodeValue !== ancestorValue)
);
});
}
142 changes: 85 additions & 57 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]) => {
describe(`with ${property}`, () => {
it(`returns true if link has ${property}`, () => {
const { linkElm, parentElm } = getLinkElm(styles);
assert.isTrue(linkInBlockStyleCheck.call(checkContext, linkElm));
assert.equal(checkContext._relatedNodes[0], parentElm);
assert.isNull(checkContext._data);
});

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);
});
});
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);
});

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
Loading