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 for cke5 to allow link attributes #13725

Merged
merged 8 commits into from
May 10, 2024

Conversation

LordRembo
Copy link
Contributor

@LordRembo LordRembo commented May 6, 2024

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #13724

Description:

This partly reenables a feature that was present in Mautic 4: allowing for more html and html properties (attributes) to be printed inside CKE.
This change is limited to:

  • attributes on Links
  • allowing the use of span tags, with attributes (used by some email template makers for special highlights in placeholder text or even to override link colors)

I also had to upgrade CKE (went from v38 to 41 ), because there was a bug that gave duplicate module errors when trying to import and add certain plugins.

When editing the contact block in the Brienz theme (and similar issues will arise in people's custom themes), the CKEditor gave a blank textarea, instead of letting you edit the existing text.
This was caused by some html attributes being in the prefilled Text Block. Instead of just the attributes being filtered out, the entire content disappeared.
Screenshot 2024-05-06 at 13 25 40
Screenshot 2024-05-06 at 13 25 57

Read the full bug report here

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a new emails
  3. Use the Brienz theme
  4. Open the contact block
  5. The content in CKE is not empty
  6. Add a bit of text, and save it
  7. Your preview shows the changed text
  8. Extra: if you inspect the Block in the Browser (using dev tools), you should see that the links (eg. for phone and email) still contain a title attribute (we don't really care for the ID or the 'draggable' attributes as they are set by the system for interaction and not by the user).
  9. Test that the other functionalities of CKE still work, eg. in another Text block: make some bold text, make a list, a link, adding a Token, …

@LordRembo LordRembo marked this pull request as ready for review May 6, 2024 12:23
@LordRembo LordRembo added ready-to-test PR's that are ready to test T1 Low difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs mautic-5 Relates to Mautic 5.x labels May 6, 2024
@LordRembo LordRembo force-pushed the fix-cke5-allow-link-attributes branch from 234fe12 to b4f9c5e Compare May 7, 2024 09:58
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I see no issues in the code. It fits the PR description, updates a lot of JS dependencies that all seem to be related to CK editor 👍

@escopecz escopecz added code-review-passed PRs which have passed code review pending-test-confirmation PR's that require one test before they can be merged editor Anything related to CKEditor and removed ready-to-test PR's that are ready to test labels May 10, 2024
@LordRembo LordRembo force-pushed the fix-cke5-allow-link-attributes branch from 8e1612d to 58963d6 Compare May 10, 2024 11:29
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.51%. Comparing base (a98a30f) to head (58963d6).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13725      +/-   ##
============================================
- Coverage     61.51%   61.51%   -0.01%     
  Complexity    34068    34068              
============================================
  Files          2241     2241              
  Lines        101852   101852              
============================================
- Hits          62651    62650       -1     
- Misses        39201    39202       +1     

see 1 file with indirect coverage changes

@abhisekmazumdar
Copy link
Contributor

Tested locally, I can confirm it resolved the issue mentioned here: #13724.

@escopecz escopecz added this to the 5.1.0 milestone May 10, 2024
@escopecz escopecz added user-testing-passed PRs which have been successfully tested by the required number of people. and removed pending-test-confirmation PR's that require one test before they can be merged labels May 10, 2024
@escopecz escopecz merged commit 3095439 into mautic:5.x May 10, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review editor Anything related to CKEditor mautic-5 Relates to Mautic 5.x T1 Low difficulty to fix (issue) or test (PR) user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Status: 🥳 Done
3 participants