Skip to content

PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor#19373

Merged
hansjovis merged 5 commits intotrunkfrom
PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor
Dec 12, 2022
Merged

PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor#19373
hansjovis merged 5 commits intotrunkfrom
PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor

Conversation

@hdvos
Copy link
Copy Markdown
Contributor

@hdvos hdvos commented Dec 9, 2022

Context

  • Fixes an unreleased bug where keyword density, keyphrase distribution and word complexity (which are all assessments that use packages/yoastseo/src/languageProcessing/helpers/word/markWordsInSentences.js for highlighting.) broke when there was an hyperlink on an item between square brackets. (<a ...>[1]</a>).
  • The bug was introduced in this PR: PC-965 Fixes highlighting when inline HTML tags are present in Classic editor and in Shopify #19162
  • In more detail: the bug was caused because, in classic editor, all items between square brackets are removed before the analysis as they are potential wordpress shortcodes.
  • NOTE: There is still an issue: not all occurrences of bread are highlighted. This is due to a known issue regarding square brackets. This could not be solved within this issue without an enourmousscope creep. There are already two JIRA issue describing this bug. PRODUCT-1041 and IM-1089.

Summary

This PR can be summarized in the following changelog entry:

  • Fixes an unreleased bug text items between square brackets that has an anchor tag breaks keyword density, keyphrase distribution and word complexity.

Relevant technical choices:

  • None

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Install and activate yoastSEO free and YoastSEO premium
  • Install and activate Classic editor
  • Have your console opened
Scenario 1: bug is not reproduced
  • Create a new post (with classic editor)
  • Paste this text in text edit mode. (see screenshot)
    Screenshot 2022-12-09 at 11 43 42
  • Make sure the following error does not occur in the console:
    yoastseo-woo-worker-154-RC1.js:1 TypeError: object null is not iterable (cannot read property Symbol(Symbol.iterator))
    
  • Add bread as your focus keyphrase.
  • Make sure that the keyword density assessment works (located in the SEO analysis).
    • It should have a green bullet.
    • It should give the following feedback: Keyphrase density: The focus keyphrase was found 3 times. This is great!
    • If you click the highlighting toggle, it highlights 2 occurrences of "bread". NOTE: it does not highlight all 3 occurrences. See the comment in the context above.
  • Make sure that the keyphrase distribution assessment works (located in the SEO analysis).
    • It should have a red bullet.
    • It should give the following feedback Keyphrase distribution: Very uneven. Large parts of your text do not contain the keyphrase or its synonyms. Distribute them more evenly.
    • If you click the highlighting toggle, it highlights 2 occurrences of "bread". NOTE: it does not highlight all 3 occurrences. See the comment in the context above.
  • Make sure that the word complexity assessment works (located in the Readability analysis).
    • It should have n orange bullet.
    • It should give the following feedback Word complexity: 17.54% of the words in your text are considered complex. Try to use shorter and more familiar words to improve readability.
    • If you click the highlighting toggle, it should highlight a lot of words, the first three highlighted words are: pounding, bread making, 14,500-year-old
  • Repeat scenario 1 for all post types (Including woo product).
    • Note: highlighting buttons are not shown in tags and categories.
  • Block editor and Elementor should not be affected, but do repeat the test above to make sure. Highlighting in Elementor can be ignored, as we do not support that.
Scenario 2: Test other types of brackets
  • Create a new post with classic editor
  • Paste this text in text edit mode.
  • Replace the square bracket pairs with parenthesis ().
  • Smoke test keyword density, keyphrase distribution and word complexity. You can use the tests from scenario 1 for this.
  • Replace the parenthesis pairs with curly brackets {}.
  • Smoke test keyword density, keyphrase distribution and word complexity. You can use the tests from scenario 1 for this.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

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:

  • This PR impacts all assessments that use the markWordsInSentences.js module. These are keyword density, keyphrase distribution and word complexity. Tests are included for all assessments.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unit tests to verify the code works as intended
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes https://yoast.atlassian.net/browse/PC-1029?atlOrigin=eyJpIjoiOTU4OWUyMDI3ZmY4NDEzYmJkOTIxNzMzZDMyOGFkZDUiLCJwIjoiaiJ9

@hdvos hdvos added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Dec 9, 2022
@hdvos hdvos marked this pull request as ready for review December 9, 2022 13:12
Copy link
Copy Markdown
Contributor

@hansjovis hansjovis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hansjovis
Copy link
Copy Markdown
Contributor

Acceptance test: ✅

@hansjovis hansjovis added this to the 19.13 milestone Dec 12, 2022
@hansjovis hansjovis merged commit 69006b0 into trunk Dec 12, 2022
@hansjovis hansjovis deleted the PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor branch December 12, 2022 11:17
@enricobattocchi enricobattocchi modified the milestones: 19.13, 19.14 Dec 13, 2022
mhkuu added a commit that referenced this pull request Dec 21, 2022
…ng-square-brackets-in-content-lead-to-errors-with-classic-editor"

This reverts commit 69006b0, reversing
changes made to 89a5291.
@manuelaugustin manuelaugustin added changelog: reverted Needs to be excluded from all categories in the changelog, should be used for reverted PR and removed changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog labels Dec 21, 2022
@hdvos hdvos restored the PC-1029-anchors-containing-square-brackets-in-content-lead-to-errors-with-classic-editor branch January 10, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: reverted Needs to be excluded from all categories in the changelog, should be used for reverted PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants