[IM] 19477 yoast markers break html of content in classic editor and shopify#19648
Conversation
…attempt-3-yoast-markers-break-html-of-content-in-shopify-and-classic
# Conflicts: # packages/yoastseo/spec/languageProcessing/helpers/sentence/SentenceTokenizerSpec.js
# Conflicts: # packages/yoastseo/spec/languageProcessing/helpers/sentence/SentenceTokenizerSpec.js # packages/yoastseo/spec/scoring/productPages/fullTextTests/testTexts/en/englishPaper2.js
…-break-html-of-content-in-shopify-and-classic
| // 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; |
There was a problem hiding this comment.
Question for CR: not sure what would be a good name for isMaybeSingleQuoted. Suggestions would be welcome.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
good suggestion. Will apply.
hdvos
added
changelog: bugfix
There was a problem hiding this comment.
It's really great that you also found a solution for the latest issue found, and a neat solution at that! And the code looks great in general. I really appreciate the efforts you were pouring into this PR 🙌🏽
I have some minor suggestions below and also some answers to your questions 😄
| // 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; |
There was a problem hiding this comment.
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)
…enceTokenizer.js Co-authored-by: Aida Marfuaty <48715883+FAMarfuaty@users.noreply.github.com>
Co-authored-by: Aida Marfuaty <48715883+FAMarfuaty@users.noreply.github.com>
hdvos
changed the title
|
This PR will be closed in favor of this PR #19688. This is because this PR contains changes from |
mhkuu
deleted the
19477-attempt-3-yoast-markers-break-html-of-content-in-shopify-and-classic
branch
3 participants
Context
catnipalso occurred in the url in the anchor).Summary
This PR can be summarized in the following changelog entry:
yoastmarktags broke the HTML when applied to inline HTML attributes.yoastmarkto anchor tag attributes.Relevant technical choices:
getSentencesFromTokens, a check was added. Previously, if there was an opening tag on the beginning of the text, and a closing tag at the end, both would be removed, regardless of whether they belonged together. In this PR a check is added that does a rudimentary (and not watertight) check whether they belong together. Once we have the html parser, this should be replaced.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
NOTE: while testing, you might note that there are different results for the same text between post and page in default editor. This is reproducible on
trunk. And this IM issue was created for itThis testing plan is quite extensive. The reason is that this PR touches A lot of different marking
WordPress
catnip.txt
NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorKeyword density assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Keyphrase distribution assessment
composer require yoast/wordpress-seo:dev-19477-attempt-3-yoast-markers-break-html-of-content-in-shopify-and-classic@devbefore building ithrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Synonyms:
catmintas a keyphrase synonym.catnipare markedcatnipas well as all occurrences ofcatmintare markedcatmintcatmintare properly highlighted.catmintare still there. (there should be 3 occurrences)Word complexity assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Sentence length, Passive voice, and Transition words assessment
However, the compounds were found to repel <a href="https://en.wikipedia.org/wiki/Mosquito" rel="nofollow">mosquitos</a>, and it is hypothesized that rubbing against the plants provides the cats with a chemical coat that protects them against mosquito bites.NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorConsecutive sentences assessment
<a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ. <a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ. <a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ.NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorParagraph length assessment
Paragraph length assessment
Inclusive language analysis
Upgrade routine
Install and activate the previous version of Yoast SEO
Set the site language to English
Create a post in Classic editor and add this text:
catnip.txt
NOTE: Add the text in
Texteditor mode in Classic editorSet "catnip" as the focus keyphrase
Embed this link below to the phrase "catnip flowers":
Smoke test Advanced custom fields
Smoke classic editor block in block editor
/classicblock (create a new block and type/classic), and add some text to it that contains the wordscatnipandcatmint.catnipandcatmintare highlighted.catnipare highlighted.Test in Shopify
Install and activate Yoast SEO for Shopify
maininshopify-seowordpress-seobefore buildingSet the store language to English
Create a product and add this text:
catnip.txt
NOTE: the test instruction below should be repeated in all content types in Shopify
Keyword density and Keyphrase distribution assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Confirm that the keyword density assessment still detects 8 occurrences of the keyphrase in the text
Click the eye icon of keyword density assessment
Confirm that the "catnip" in "catnip flower" is highlighted
Check the anchor link of the phrase and confirm that the
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Click the eye icon of keyphrase distribution assessment
Confirm that the "catnip" in "catnip flowers" is highlighted
Check the anchor link of the phrase and confirm that the
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Word complexity assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"(Smoke) Test impact by sentence tokenizer
<i>The cat</i> was greeted by <i>the dog</i>.<div>The cat was greeted by the dog</div>.midgetto a few random sentences in the post. This should trigger the inclusive language assessment formidget.midget.Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel and noted the work hours.Fixes #19477