Skip to content
Closed
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
779d541
add check for advanced custom fields
hdvos Jan 10, 2023
ddb5a77
Merge branch 'trunk' into PC-965-yoast-markers-break-html-of-content
hdvos Jan 10, 2023
874438c
Merge branch 'PC-965-yoast-markers-break-html-of-content' into 19477-…
hdvos Jan 10, 2023
f462016
reapply PC-965
hdvos Jan 10, 2023
20f462e
re-apply PC 1029
hdvos Jan 10, 2023
009ff23
semtence tokenizer fix
hdvos Jan 11, 2023
21a3c9c
add jsdoc
hdvos Jan 11, 2023
3e1b9ed
modify extra check for removing outer html tags
hdvos Jan 12, 2023
aa26fe7
update isValidPair check
hdvos Jan 12, 2023
47ce9e8
add specs for isValidPair
hdvos Jan 12, 2023
ac3c71e
rename function
hdvos Jan 12, 2023
9dbd692
adapt specs
hdvos Jan 12, 2023
03fe68c
adapt specs
hdvos Jan 12, 2023
88bcbc2
solve non breaking space problem in replaceQuotes
hdvos Jan 17, 2023
56ca5aa
adapt replaceQuotesSpec
hdvos Jan 17, 2023
890edce
rename replaceQuotes
hdvos Jan 17, 2023
32843dc
adapt fulltesttexts
hdvos Jan 17, 2023
ca19e85
commit snapshots
hdvos Jan 17, 2023
bffd1e3
Merge branch 'feature/lingo-fixes' into 19477-attempt-3-yoast-markers…
hdvos Jan 17, 2023
4806b48
fix linting error
hdvos Jan 17, 2023
abbd101
fix marking of gutenberg.
hdvos Jan 18, 2023
69b7016
refine isValidTagPair
hdvos Jan 19, 2023
c443dae
update comment
hdvos Jan 19, 2023
42ef420
rename replaceQuotesSpec to normalizeHTMLSpec
hdvos Jan 19, 2023
2f90b0d
adapt fulltextTests
hdvos Jan 19, 2023
40d46f2
Update packages/yoastseo/src/languageProcessing/helpers/sentence/Sent…
hdvos Jan 20, 2023
9ab066d
Update packages/js/src/decorator/tinyMCE.js
hdvos Jan 20, 2023
51e4745
apply CR comments
hdvos Jan 20, 2023
eeede89
add spec for normalizeHTML when an nbsp is present
hdvos Jan 20, 2023
5c0c8ae
rename isMaybeSingleQuoted to doesNotContainDoubleQuotedMark
hdvos Jan 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
30 changes: 27 additions & 3 deletions packages/js/src/decorator/gutenberg.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import { create } from "@wordpress/rich-text";
import { select, dispatch } from "@wordpress/data";
import getFieldsToMarkHelper from "./helpers/getFieldsToMarkHelper";


const ANNOTATION_SOURCE = "yoast";

export const START_MARK = "<yoastmark class='yoast-text-mark'>";
const START_MARK_DOUBLE_QUOTED = "<yoastmark class=\"yoast-text-mark\">";
export const END_MARK = "</yoastmark>";

let annotationQueue = [];
Expand Down Expand Up @@ -144,6 +144,18 @@ export function isAnnotationAvailable() {
*/
export function getYoastmarkOffsets( marked ) {
let startMarkIndex = marked.indexOf( START_MARK );

// Checks if the start mark is single quoted.
// Note: if isMaybeSingleQuoted is true, this does nessarily mean that the start mark is single quoted.
// It could also be that the start mark doesn't occur at all in startMarkIndex.
// In that case, startMarkIndex will be -1 during later tests.
const isMaybeSingleQuoted = startMarkIndex >= 0;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Question for CR: not sure what would be a good name for isMaybeSingleQuoted. Suggestions would be welcome.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think isStartMarkSingleQuoted would be more appropriate? because when it's true, we know for sure that it is single quoted (so not a "maybe" anymore)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason I went with Maybe is because of the following note
// Note: if isMaybeSingleQuoted is true, this does necessary mean that the start mark is single quoted.

if startMarkIndex >= 0, this means that the doublequoted start mark is not present. This could either mean that the start mark is single quoted, or that no start mark occurs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I think I misunderstood the comment then.
So, there are three cases: no start mark, single quote, and double quote. startMarkIndex >= 0 is true for single quote and false for double quote, but is it supposed to be true or false for no start mark occurs?
But, if it is also true for no start mark, wouldn't it be appropriate to call the variable isNotDoubleQuoted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good suggestion. Will apply.


// If the start mark is not found, try the double quoted version.
if ( ! isMaybeSingleQuoted ) {
startMarkIndex = marked.indexOf( START_MARK_DOUBLE_QUOTED );
}

let endMarkIndex = null;

const offsets = [];
Expand All @@ -154,7 +166,12 @@ export function getYoastmarkOffsets( marked ) {
* without the tags.
*/
while ( startMarkIndex >= 0 ) {
marked = marked.replace( START_MARK, "" );
if ( isMaybeSingleQuoted ) {
marked = marked.replace( START_MARK, "" );
} else {
marked = marked.replace( START_MARK_DOUBLE_QUOTED, "" );
}
// marked = marked.replace( START_MARK, "" );
Comment thread
hdvos marked this conversation as resolved.
Outdated
endMarkIndex = marked.indexOf( END_MARK );

if ( endMarkIndex < startMarkIndex ) {
Expand All @@ -167,7 +184,12 @@ export function getYoastmarkOffsets( marked ) {
endOffset: endMarkIndex,
} );

startMarkIndex = marked.indexOf( START_MARK );
if ( isMaybeSingleQuoted ) {
startMarkIndex = marked.indexOf( START_MARK );
} else {
startMarkIndex = marked.indexOf( START_MARK_DOUBLE_QUOTED );
}
// startMarkIndex = marked.indexOf( START_MARK );
Comment thread
hdvos marked this conversation as resolved.
Outdated
endMarkIndex = null;
}

Expand Down Expand Up @@ -220,6 +242,7 @@ export function calculateAnnotationsForTextFormat( text, mark ) {
*
* A cool <b>keyword</b>. => A cool keyword.
*/

const originalSentence = mark.getOriginal().replace( /(<([^>]+)>)/ig, "" );

/*
Expand Down Expand Up @@ -452,6 +475,7 @@ export function applyAsAnnotations( marks ) {
blocks = blocks.filter( block => fieldsToMark.some( field => "core/" + field === block.name ) );
}


const annotations = getAnnotationsForBlocks( blocks, marks );

fillAnnotationQueue( annotations );
Expand Down
22 changes: 20 additions & 2 deletions packages/js/src/decorator/tinyMCE.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { markers } from "yoastseo";
import { markers, languageProcessing } from "yoastseo";
import { forEach } from "lodash-es";
import { languageProcessing } from "yoastseo";

var MARK_TAG = "yoastmark";

Expand Down Expand Up @@ -36,11 +35,30 @@ function markTinyMCE( editor, paper, marks ) {
let html = editor.getContent();
html = markers.removeMarks( html );

/*
* Get the information whether we want to mark a specific part of the HTML. If we do, `fieldsToMark` should return an array with that information.
* For example, [ "heading" ] means that we want to apply the markings in subheadings only, and not the other parts.
* `selectedHTML` is an array of the HTML parts that we want to apply the marking to.
*/
const { fieldsToMark, selectedHTML } = languageProcessing.getFieldsToMark( marks, html );

// Generate marked HTML.
forEach( marks, function( mark ) {
/*
* Classic editor uses double quotes for HTML attribute values. However, Block editor uses single quotes for HTML tag attributes,
* and that's why in `yoastseo`, we use single quotes for the attribute values when we create the marked object. As a result,
* the replacement did not work, as the marks passed by `yoastseo` did not match anything in the original text.
* This step is replacing the single quotes in the marked object output by `yoastseo` with double quotes.
* This way, we make sure that the replacement can find a match between the original text of the marked object and the text in the page.
*/
if ( editor.id !== "acf_content" ) {
mark._properties.marked = languageProcessing.normalizeHTML( mark._properties.marked );
mark._properties.original = languageProcessing.normalizeHTML( mark._properties.original );
}

// Check if we want to mark only specific part of the HTML.
if ( fieldsToMark.length > 0 ) {
// Apply the marking to the selected HTML parts.
selectedHTML.forEach( element => {
const markedElement = mark.applyWithReplace( element );
html = html.replace( element, markedElement );
Expand Down
4 changes: 2 additions & 2 deletions packages/js/tests/settings/__snapshots__/search.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

exports[`Search test should match search snapshot 1`] = `
"<Fragment>
<button type=\\"button\\" className=\\"yst-w-full yst-flex yst-items-center yst-bg-white yst-text-sm yst-leading-6 yst-text-slate-500 yst-rounded-md yst-border yst-border-slate-300 yst-shadow-sm yst-py-1.5 yst-pl-2 yst-pr-3 focus:yst-outline-none focus:yst-ring-2 focus:yst-ring-offset-2 focus:yst-ring-primary-500\\" onClick={[Function (anonymous)]}>
<button id=\\"button-search\\" type=\\"button\\" className=\\"yst-w-full yst-flex yst-items-center yst-bg-white yst-text-sm yst-leading-6 yst-text-slate-500 yst-rounded-md yst-border yst-border-slate-300 yst-shadow-sm yst-py-1.5 yst-pl-2 yst-pr-3 focus:yst-outline-none focus:yst-ring-2 focus:yst-ring-offset-2 focus:yst-ring-primary-500\\" onClick={[Function (anonymous)]}>
<ForwardRef(SearchIcon) className=\\"yst-flex-none yst-w-5 yst-h-5 yst-mr-3 yst-text-slate-400\\" role=\\"img\\" aria-hidden=\\"true\\" />
<span className=\\"yst-overflow-hidden yst-whitespace-nowrap yst-text-ellipsis\\">
Quick search...
</span>
</button>
<Modal onClose={[Function (anonymous)]} isOpen={false} initialFocus={{...}} position=\\"top-center\\">
<Modal onClose={[Function (anonymous)]} isOpen={false} initialFocus={{...}} position=\\"top-center\\" aria-label=\\"Search\\">
<Modal.Panel closeButtonScreenReaderText=\\"Close\\">
<ComboboxFn as=\\"div\\" className=\\"yst--m-6 yst--mt-5\\" onChange={[Function (anonymous)]}>
<div className=\\"yst-relative\\">
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,9 @@ const expectedResults = {
},
keyphraseDistribution: {
isApplicable: true,
score: 9,
resultText: "<a href='https://yoa.st/33q' target='_blank'>Keyphrase distribution</a>: Good job!",
score: 6,
resultText: "<a href='https://yoa.st/33q' target='_blank'>Keyphrase distribution</a>: Uneven. Some parts of your text do not contain " +
"the keyphrase or its synonyms. <a href='https://yoa.st/33u' target='_blank'>Distribute them more evenly</a>.",
},
subheadingsTooLong: {
isApplicable: true,
Expand All @@ -121,7 +122,7 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 18.6% of the sentences contain transition words, " +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 17.8% of the sentences contain transition words, " +
"which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 6,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 27.1% of the sentences contain " +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 25.5% of the sentences contain " +
"transition words, which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 16.7% of the sentences contain passive voice, " +
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 15.7% of the sentences contain passive voice, " +
"which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>" +
"Try to use their active counterparts</a>.",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,13 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 19.7% of the sentences contain" +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 19.4% of the sentences contain" +
" transition words, which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 25.9% of the sentences contain passive voice, " +
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 25.5% of the sentences contain passive voice, " +
"which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>" +
"Try to use their active counterparts</a>.",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 5.6% of the sentences contain transition words, " +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 5.5% of the sentences contain transition words, " +
"which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ const expectedResults = {
passiveVoice: {
isApplicable: true,
score: 6,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 11.8% of the sentences contain passive voice, " +
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 11.5% of the sentences contain passive voice, " +
"which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>" +
"Try to use their active counterparts</a>.",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 13.1% of the sentences contain transition words, which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 12.5% of the sentences contain transition words, which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
isApplicable: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ const expectedResults = {
passiveVoice: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 25.3% of the sentences contain passive voice, which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>Try to use their active counterparts</a>.",
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 24.7% of the sentences contain passive voice, which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>Try to use their active counterparts</a>.",
},
textPresence: {
isApplicable: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,8 @@ const expectedResults = {
},
passiveVoice: {
isApplicable: true,
score: 6,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 10.6% of the sentences contain passive voice, " +
"which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>" +
"Try to use their active counterparts</a>.",
score: 9,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: You're using enough active voice. That's great!",
},
textPresence: {
isApplicable: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 9.5% of the sentences contain transition words," +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 9% of the sentences contain transition words," +
" which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 16.1% of the sentences contain passive voice, " +
resultText: "<a href='https://yoa.st/34t' target='_blank'>Passive voice</a>: 15.3% of the sentences contain passive voice, " +
"which is more than the recommended maximum of 10%. <a href='https://yoa.st/34u' target='_blank'>" +
"Try to use their active counterparts</a>.",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ const expectedResults = {
textTransitionWords: {
isApplicable: true,
score: 3,
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 13.3% of the sentences contain transition words," +
resultText: "<a href='https://yoa.st/34z' target='_blank'>Transition words</a>: Only 12.8% of the sentences contain transition words," +
" which is not enough. <a href='https://yoa.st/35a' target='_blank'>Use more of them</a>.",
},
passiveVoice: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,10 @@ const expectedResults = {
isApplicable: false,
},
keyphraseDistribution: {
// The text doesnt contain more than 15 sentences.
isApplicable: false,
isApplicable: true,
score: 6,
resultText: "<a href='https://yoa.st/33q' target='_blank'>Keyphrase distribution</a>: Uneven. Some parts of your text do not " +
"contain the keyphrase or its synonyms. <a href='https://yoa.st/33u' target='_blank'>Distribute them more evenly</a>.",
},
subheadingsTooLong: {
isApplicable: true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import normalizeHTML from "../../../../src/languageProcessing/helpers/html/normalizeHTML";

describe( "replace-quotes", function() {
describe( "normalizeHTML", function() {
it( "should return the same string when no single quotes are present", function() {
expect( normalizeHTML( "This is a test" ) )
.toEqual( "This is a test" );
} );

it( "should return the same string when only double quotes in HTML attribute values are present", function() {
expect( normalizeHTML( "<yoastmark class=\"yoast-text-mark\">This is a test</yoastmark>" ) )
.toEqual( "<yoastmark class=\"yoast-text-mark\">This is a test</yoastmark>" );
} );

it( "should not replace single quotes (or apostrophes) outside HTML tags", function() {
expect( normalizeHTML( "This is a test, let's go!" ) )
.toEqual( "This is a test, let's go!" );
} );

it( "should replace the outer single quotes in HTML attribute values with double quotes", function() {
expect( normalizeHTML( "<span style='color: red'>This</span> is a test" ) )
.toEqual( "<span style=\"color: red\">This</span> is a test" );
} );

it( "should not replace any inner single quotes in HTML attribute values", function() {
expect( normalizeHTML( "<span data-attr=\"let's go, time's up\">This</span> is a test" ) )
.toEqual( "<span data-attr=\"let's go, time's up\">This</span> is a test" );
} );

it( "should replace the outer single quotes in multiple HTML attribute values with double quotes", function() {
expect( normalizeHTML( "<yoastmark class='yoast-text-mark' style='color: blue'>This is a test</yoastmark>" ) )
.toEqual( "<yoastmark class=\"yoast-text-mark\" style=\"color: blue\">This is a test</yoastmark>" );
} );
} );
} );
Comment thread
hdvos marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,38 @@ describe( "A test for tokenizing a (html) text into sentences", function() {
} );
} );


describe( "testing the isValidTagPair helper method", function() {
it( "returns true if the tags are of the same type and the correct type", function() {
[ "p", "div", "h1", "h2", "h3", "h4", "h5", "h6", "span" ].forEach( function( tagType ) {
const mockOpenTag = { src: `<${tagType}>` };
const mockCloseTag = { src: `</${tagType}>` };
expect( mockTokenizer.isValidTagPair( mockOpenTag, mockCloseTag ) ).toBe( true );
} );
} );
it( "returns true if the tags are of the same type and the correct type and the have attributes", function() {
[ "p", "div", "h1", "h2", "h3", "h4", "h5", "h6", "span" ].forEach( function( tagType ) {
const mockOpenTag = { src: `<${tagType} class="onzin" messy="1">` };
const mockCloseTag = { src: `</${tagType}>` };
expect( mockTokenizer.isValidTagPair( mockOpenTag, mockCloseTag ) ).toBe( true );
} );
} );
it( "returns false if the tags are of the same type but not of the correct type", function() {
const mockOpenTag = { src: "<i>" };
const mockCloseTag = { src: "</i>" };
expect( mockTokenizer.isValidTagPair( mockOpenTag, mockCloseTag ) ).toBe( false );
} );
it( "returns false if the tags are of te correct type but not of the same type", function() {
const mockOpenTag = { src: "<div>" };
const mockCloseTag = { src: "</span>" };
expect( mockTokenizer.isValidTagPair( mockOpenTag, mockCloseTag ) ).toBe( false );
} );
it( "returns false if the tags are of te wrong type and not of the same type", function() {
const mockOpenTag = { src: "<i>" };
const mockCloseTag = { src: "</b>" };
expect( mockTokenizer.isValidTagPair( mockOpenTag, mockCloseTag ) ).toBe( false );
} );
} );
describe( "Tests for isBreakTag", () => {
it( "returns true for a break tag", () => {
expect( mockTokenizer.isBreakTag( "<br" ) ).toBe( true );
Expand Down
Loading