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

Provide a filter to parse shortcodes #20812

Merged
merged 41 commits into from
Nov 20, 2023

Conversation

FAMarfuaty
Copy link
Contributor

@FAMarfuaty FAMarfuaty commented Oct 31, 2023

Context

  • Per this PR: 20508 html parser change how and where shortcodes are removed #20528, we decided not to parse shortcodes that enter Paper._text for several reasons, namely:

    • This way we can recognize shortcodes more accurately by comparing the shortcodes array $shortcodes global variable and the unparsed shortcodes inside Paper._text.
    • This way we can retrieve the position information of the tokens in the text more accurately. Accurate position information means accurate position-based highlighting.
      • The position-based highlighting feature refers to the token position in the editor. This means that if a shortcode is present, the position is relative to the unparsed shortcode. Parsing the shortcode would mean messing up with the position information of the tokens surrounding it.
    • We didn't parse the shortcodes because we always exclude them from our analysis.
  • With this PR, we provide a filter to add an array of shortcodes that a user wants us to parse. Parsed shortcodes mean that we will include them in our analysis. ⚠️ However, it must be noted that parsing the shortcodes means that it's highly likely that we won't be able to highlight text accurately, for the reasons mentioned above. For this reason, we disable/grey out the highlighting button when the shortcodes are parsed.

Summary

This PR can be summarized in the following changelog entry:

  • Provides a filter to add a list of shortcodes so they can be parsed and then included in our content analysis in Classic editor.

Relevant technical choices:

  • The Yoast shortcode plugin only parses the shortcodes in Classic editor.
  • When the Yoast shortcode plugin is used, the highlighting (eye) button is greyed out in Classic editor
  • The Yoast shortcode plugin parses the shortcodes before the analysis is run. The steps is as follows:
    • When the list of shorcodes to be parsed is available, we activate the Yoast shortcode plugin upon initializing the post scrapper
    • The shortcode plugin parses the shortcodes and replaces the unparsed ones with the parsed ones in the text
    • The modified text ("content") with unparsed shortcodes is registered via registerModification API of packages/yoastseo/src/app.js, which is a wrapper of registerModification function of Pluggable plugin. Pluggable plugin takes care of plugin registrations, preloading and managing data modifications. (see packages/yoastseo/src/pluggable.js)
    • Before we send the analysis data to the Paper (see packages/js/src/analysis/collectAnalysisData.js), we apply the modifications that we previously registered. This includes the modification of the text/content where the shortcodes are parsed.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Add the shortcodes for testing

Test in a post WITH shortcodes for parsing available

  • Apply Add the shortcodes for testing above.
  • Install and activate Yoast SEO
  • Install and activate Yoast SEO Premium
  • Install and activate Classic editor
  • Set the site language to English
  • Toggle on the Insights feature card in Yoast SEO setting
  • Enable Inclusive language analysis in the setting
  • Create a post in Classic editor
  • Set the focus keyphrase to: cats
  • Add some text more than 300 words
  • Add a gallery shortcode: [gallery] using the Text editor
  • Switch back to Visual editor
  • Click/edit on the added gallery
    • This way you can add multiple images to the gallery:
Screenshot 2023-11-09 at 10 54 36
  • Add two images to the gallery. Include the focus keyphrase in the caption of BOTH images.
  • Confirm that the Images assessment returns a 🟢 traffic light.
  • Confirm that the Image Keyphrase assessment returns a 🟢 traffic light.
  • 🗒️ Note that Keyphrase density assessment does not recognize the keyphrase in the image caption inside the gallery, this is a known issue.
  • Remove the gallery shortcode.
  • Install and activate Shortcodes Ultimate plugin.
  • Add a gallery shortcode from the Shortcodes Ultimate plugin:
Screenshot 2023-11-09 at 11 04 47 Screenshot 2023-11-09 at 11 05 06
  • Add two images to the gallery. Include the focus keyphrase in the caption of BOTH images.
  • Confirm that the Images assessment gives a 🟢 traffic light.
  • Confirm that the Image Keyphrase assessment returns a 🟠 traffic light (this plugin handles alt tags incorrectly).
  • Open the Insights collapsible.
  • Confirm that the Word count inside Insights is the same as the feedback of Text length assessment.
  • Save your changes and confirm that the analysis results stay the same.

Check the highlighting button

  • Apply Add the shortcodes for testing above.
  • Open the post created in above section
  • Add a passive voice sentence to the text
  • Add a non-inclusive phrase to the text: My cats are going crazy..
  • In the post created above confirm the following:
    • The highlighting (eye) icons are greyed out in SEO analysis, e.g. check Keyphrase density assessment.
    • The highlighting (eye) icons are greyed out in SEO analysis, e.g. check Passive voice assessment.
    • The highlighting (eye) icons are greyed out in Inclusive language analysis.
  • Save your changes.

Test in a post WITHOUT shortcodes for parsing available

  • Undo the changes done in Add the shortcodes for testing above.
  • Go back to the post that was previously created
  • Confirm that the Images assessment gives a 🔴 traffic light: now it doesn't recognize the images inside the gallery
  • In the post created above confirm the following:
    • The highlighting (eye) icons are NOT greyed out in SEO analysis, e.g. check Keyphrase density assessment.
    • The highlighting (eye) icons are NOT greyed out in SEO analysis, e.g. check Passive voice assessment.
    • The highlighting (eye) icons are NOT greyed out in Inclusive language analysis.

Test in a taxonomy

🗒️ Note that highlighting is unavailable in taxonomies by default. Note that we also have no image assessments for taxonomies. So, we'll test a little bit differently:

  • Apply Add the shortcodes for testing above.
  • Create a new taxonomy.
  • Add some text.
  • Add the shortcode [su_dummy_text] from the Shortcodes Ultimate plugin.
  • Confirm that the text length does not increase, not in the text length assessment and also not in the Insights section.
  • Now edit your MyShortcodesFilter.js and add su_dummy_text to the allowed shortcodes, so that it becomes shortcodes.push( "gallery", "su_custom_gallery", "su_dummy_text" );.
  • Repeat the above steps.
  • Confirm that now the text length does increase, both in the text length assessment and the Insights section.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/custom post types
  • 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:

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.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • 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.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes #20801

@FAMarfuaty FAMarfuaty changed the base branch from feature/lingo-fixes to trunk October 31, 2023 10:35
@FAMarfuaty FAMarfuaty linked an issue Oct 31, 2023 that may be closed by this pull request
11 tasks
@coveralls
Copy link

coveralls commented Oct 31, 2023

Pull Request Test Coverage Report for Build 6862911366

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 4 (0.0%) changed or added relevant lines in 2 files are covered.
  • 555 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 49.293%

Changes Missing Coverage Covered Lines Changed/Added Lines %
admin/metabox/class-metabox.php 0 2 0.0%
src/integrations/third-party/elementor.php 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
src/generated/container.php 555 0.0%
Totals Coverage Status
Change from base Build 6809766998: 0.006%
Covered Lines: 13246
Relevant Lines: 26872

💛 - Coveralls

@FAMarfuaty FAMarfuaty added the changelog: other Needs to be included in the 'Other' category in the changelog label Nov 8, 2023
@FAMarfuaty FAMarfuaty marked this pull request as ready for review November 10, 2023 11:03
…ble. Otherwise, initiate a new Pluggable plugin. This is because in Elementor, pluggable is not available in the window
… watching the content retrieved from the main text editor
Copy link
Contributor

@mhkuu mhkuu left a comment

Choose a reason for hiding this comment

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

CR: Awesome work! 🏆 I've added some comments to address some cosmetic changes. I know that we reinstate the shortcode-plugin.js file, so they're not on you, but I think it's a good chance to clean it up now so future maintainers will thank you later. Otherwise, the code looks great to me, and super nice find on the Insights tab. Am I correct that we could consider moving that into a separate PR?

packages/js/src/analysis/plugins/shortcode-plugin.js Outdated Show resolved Hide resolved
packages/js/src/analysis/plugins/shortcode-plugin.js Outdated Show resolved Hide resolved
packages/js/src/elementor/initializers/pluggable.js Outdated Show resolved Hide resolved
packages/js/src/initializers/post-scraper.js Outdated Show resolved Hide resolved
packages/js/src/insights/initializer.js Outdated Show resolved Hide resolved
packages/js/src/analysis/plugins/shortcode-plugin.js Outdated Show resolved Hide resolved
packages/js/src/analysis/plugins/shortcode-plugin.js Outdated Show resolved Hide resolved
packages/js/src/analysis/plugins/shortcode-plugin.js Outdated Show resolved Hide resolved
@mhkuu
Copy link
Contributor

mhkuu commented Nov 17, 2023

We're currently only supporting shortcode parsing for posts, not taxonomies. Let's check how easy it is to support taxonomies as well. Test instructions need to be adapted slightly as we don't have the Images assessment for taxonomies (and we also don't have highlighting there). If it turns out to be too complex we can offload this to a separate IM issue.

Copy link
Contributor Author

@FAMarfuaty FAMarfuaty left a comment

Choose a reason for hiding this comment

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

@mhkuu I had a look at the last two commits and I think the changes are all good! 🙌🏽 It's a good decision to move the logic for initializing Yoast shortcode plugin out of post-scrapper.js. That way we avoid code duplication :)

@mhkuu
Copy link
Contributor

mhkuu commented Nov 20, 2023

ACC: We confirmed together things work as supposed! 🎉

@mhkuu mhkuu added this to the 21.7 milestone Nov 20, 2023
@mhkuu mhkuu merged commit 3f0c19b into trunk Nov 20, 2023
34 checks passed
@mhkuu mhkuu deleted the 20801-incorrect-analysis-results-when-parsing-shortcode branch November 20, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: other Needs to be included in the 'Other' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect analysis results when parsing shortcode
3 participants