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

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 24, 2023

First of 2 for #4126 allowing link-in-text-block to use an ancestor inline element's styles when considering the style of the link. While doing this I noticed that the unit tests didn't test any of the styles we consider to be distinct, so went ahead and added them.

@straker straker requested a review from a team as a code owner August 24, 2023 15:33
@@ -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.


const fixture = document.getElementById('fixture');
function createTestFixture({ target, root, inline } = {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some cleanup of this file. It didn't need to create actual style sheets for the elements so just made a simpler function to create inline styles.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

It seems I forgot to press "submit review" last time. Some of these are from a week ago. Sorry about that.

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?

lib/commons/color/element-is-distinct.js Show resolved Hide resolved
Comment on lines 43 to 44
const borderClr = new Color();
borderClr.parseString(curNodeStyle.getPropertyValue(edge + '-color'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be nice if we had this:

Suggested change
const borderClr = new Color();
borderClr.parseString(curNodeStyle.getPropertyValue(edge + '-color'));
const borderClr = Color.fromString(curNodeStyle.getPropertyValue(edge + '-color'));

Not related to the PR. Just an idea.

lib/commons/color/element-is-distinct.js Show resolved Hide resolved
lib/commons/color/element-is-distinct.js Outdated Show resolved Hide resolved
lib/commons/color/element-is-distinct.js Outdated Show resolved Hide resolved
test/checks/color/link-in-text-block-style.js Outdated Show resolved Hide resolved
test/commons/color/element-is-distinct.js Outdated Show resolved Hide resolved
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.

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?

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.

'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.

}

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?

@dequejenn dequejenn marked this pull request as draft November 1, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants