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

Fix #866: Add element existence check to prevent Page Builder editor crashes #873

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

artttj
Copy link

@artttj artttj commented Aug 30, 2024

Description

We encountered a scenario where missing HTML elements in Page Builder led to runtime errors due to direct calls to setAttribute.

To address this, we added an existence check before setting attributes. This change prevents the editor from breaking and improves stability by handling cases where elements are missing.

Ideally, the core should include such checks to enhance overall resilience and user experience.

Story

N/A

Bug

N/A

Task

N/A

Fixed Issues

  1. Graceful Handling of Null element in Page Builder to Prevent Editor Failure #866: Graceful Handling of Null element in Page Builder to Prevent Editor Failure

Builds

N/A

Related Pull Requests

N/A

Manual Testing Scenarios

Please test the following scenarios to verify the changes:

  1. Remove data-pb-style Attribute:

    • Remove the data-pb-style attribute from an element with applied styles.
    • Ensure the Page Builder continues to work without issues.
  2. Modify data-pb-style Hash:

    • Change the data-pb-style attribute's value to a non-relevant hash.
    • Confirm that the Page Builder handles this gracefully and remains functional.
  3. Remove HTML Element:

    • Delete the HTML element to which styles are applied.
    • Check that the Page Builder does not crash and continues to operate normally.

Note: This issue was observed when using the DeepL API, but similar problems might occur in other scenarios. The preview editor should remain stable.

Questions or comments

N/A

Checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@artttj
Copy link
Author

artttj commented Aug 30, 2024

@magento run all tests

@artttj artttj changed the title magento/magento2-page-builder#866: Add element existence check to prevent Page Builder editor crashes Fix #866: Add element existence check to prevent Page Builder editor crashes Aug 30, 2024
@artttj artttj force-pushed the fix/866_add_element_existence_check branch from 300cae8 to fab562f Compare September 9, 2024 19:23
@artttj artttj force-pushed the fix/866_add_element_existence_check branch from fab562f to c5287ab Compare September 21, 2024 22:51
@artttj artttj force-pushed the fix/866_add_element_existence_check branch from c5287ab to 67be2a5 Compare September 29, 2024 22:47
Encountered a scenario where missing HTML elements in Page Builder caused runtime errors due to direct calls to setAttribute.

Added an existence check before setting attributes to avoid breaking the editor.

This ensures continued functionality and improves stability by handling cases where elements are not found.

Logically, the core should include such checks to enhance resilience and user experience.
@engcom-Hotel
Copy link
Collaborator

@magento run all tests

Copy link
Collaborator

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @artttj,

Thanks for the contribution!

The PR changes seems good to me, but please cover the changes with some automated test in accordance of DOD.

Thanks

@engcom-Hotel
Copy link
Collaborator

After discussing it internally, it was found that the automated test is not required for this change. Hence moving it to ready for testing.

Thanks

@engcom-Hotel
Copy link
Collaborator

Hello @artttj,

Thanks for the contribution!

❌ QA not Passed

We have tried to reproduce the actual issue in the 2.4-develop branch with pagebuilder's develop branch, but the issue is not reproducible for us. Please refer to the below screencast for reference:

Screen.Recording.2024-10-25.at.5.mp4

Let us know if we missed anything.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

3 participants