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(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
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 4 additions & 4 deletions lib/checks/color/link-in-text-block-style-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ export default function linkInTextBlockStyleEvaluate(node) {

this.relatedNodes([parentBlock]);

for (const inlineNode of inlineNodes) {
if (elementIsDistinct(inlineNode, parentBlock)) {
return true;
}
if (elementIsDistinct(node, parentBlock)) {
return true;
}

for (const inlineNode of inlineNodes) {
if (hasPseudoContent(inlineNode)) {
straker marked this conversation as resolved.
Show resolved Hide resolved
this.data({ messageKey: 'pseudoContent' });
return undefined;
Expand Down
85 changes: 53 additions & 32 deletions lib/commons/color/element-is-distinct.js
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
Loading
Loading