Skip to content
This repository has been archived by the owner on Oct 4, 2022. It is now read-only.

409 improve tree comment processing #421

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,7 @@ describe( "calculateTextIndices", () => {
expect( formattingElement.textStartIndex ).toEqual( 10 );
expect( formattingElement.textEndIndex ).toEqual( 10 );
} );
} );

describe.skip( "These tests are currently broken, will be fixed in https://github.com/Yoast/javascript/issues/409", () => {
it( "correctly processes comments before another tag", () => {
const source = "<p>This is a <!--Here is a comment!--><b>paragraph</b></p>";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,23 @@ const handleIgnoredContent = function( element, html, currentOffset ) {
/**
* Sets the start and end text positions of a comment.
*
* @param {module:parsedPaper/structure.FormattingElement} element The formatting element to assign start and end text positions to.
* @param {module:parsedPaper/structure.FormattingElement} comment The formatting element to assign start and end text positions to.
* @param {int} currentOffset A sum of all characters in the source code that don't get rendered
* (e.g., tags, comments).
*
* @returns {void}
* @returns {number} The new current offset after taking this comment into account.
*
* @private
*/
const computeCommentStartEndTextIndices = function( element, currentOffset ) {
element.textStartIndex = element.location.startOffset - currentOffset;
element.textEndIndex = element.textStartIndex;
const computeCommentStartEndTextIndices = function( comment, currentOffset ) {
comment.textStartIndex = comment.location.startOffset - currentOffset;
comment.textEndIndex = comment.textStartIndex;

const length = comment.location.endOffset - comment.location.startOffset;

const newOffset = currentOffset + length;

return newOffset;
};

/**
Expand Down Expand Up @@ -165,7 +171,7 @@ const calculateTextIndices = function( node, html ) {

// Comments are self-closing formatting elements that are completely ignored in rendering.
if ( element.tag === "comment" ) {
computeCommentStartEndTextIndices( element, currentOffset );
currentOffset = computeCommentStartEndTextIndices( element, currentOffset );
return;
}

Expand All @@ -177,9 +183,9 @@ const calculateTextIndices = function( node, html ) {
}

/*
If this element is an ignored element its contents are not in the text,
so its content should be added to the respective formatting element instead,
and the current offset should be updated.
* If this element is an ignored element its contents are not in the text,
* so its content should be added to the respective formatting element instead,
* and the current offset should be updated.
*/
if ( ignoredHtmlElements.includes( element.tag ) ) {
currentOffset = handleIgnoredContent( element, html, currentOffset );
Expand Down
31 changes: 28 additions & 3 deletions packages/yoastseo/src/parsedPaper/build/tree/html/TreeAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,35 @@ class TreeAdapter {
}

/*
Some node types do not have children (like Paragraph and Heading),
but parse5 always expects a node to have children.
* Some node types do not have children (like Paragraph and Heading),
* but parse5 always expects a node to have children.
*/
return node.children || [];
let children = node.children || [];

/*
* This code is because of the following scenario:
*
* ```html
* <p>This is a <b><!--Here is a comment!-->paragraph</b></p>
* ```
*
* In that case parse5 assumes that the comment is a child of the <b>,
* but that will not happen because of our TreeAdapter. That's why we
* need to return the formatting elements after the `node` if we
* determine that the node is a formatting element.
*/
if ( children.length === 0 && node instanceof FormattingElement ) {
const formattingElement = node;

const container = formattingElement.parent;
const parentFormatting = container.textContainer.formatting;

const elementIndex = parentFormatting.indexOf( formattingElement );

children = parentFormatting.slice( elementIndex + 1 );
}

return children;
}

/**
Expand Down