Skip to content

Allow ID and data-* tags in TinyMCE #11053

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

Closed
wants to merge 1 commit into from
Closed

Allow ID and data-* tags in TinyMCE #11053

wants to merge 1 commit into from

Conversation

PieterCappelle
Copy link
Contributor

@PieterCappelle PieterCappelle commented Sep 26, 2017

Allow ID-tag and data-* tag in HTML.

Fixed Issues (if relevant)

  1. Cant put id or data attribute on <img> tags in the M2 wysiwyg. #10353

Manual testing scenarios

  1. CMS Pages / Blocks
  2. Add image
  3. Edit HTML
  4. Add id-tag or data-*="tag"
  5. Error will throw up

Contribution 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)
  • All automated tests passed successfully (all builds on Travis CI are green)

@osrecio
Copy link
Member

osrecio commented Sep 26, 2017

Same issue solved in this PR #11025

I will test your solution ASAP.

@orlangur
Copy link
Contributor

Why do you need ID attribute? It is forbidden for virtually any purpose, see http://devdocs.magento.com/guides/v2.2/coding-standards/code-standard-demarcation.html for details.

@PieterCappelle
Copy link
Contributor Author

Why should ID be forbidden? If I want specifiek markup for one element.

@orlangur
Copy link
Contributor

Because unique element today may become nonunique tomorrow. For example, #login on some pages occur more than once. Anyway, such discussion is out of scope for this PR, there is a standard which must be followed.

Please recreate branch from 2.2-develop so that it contains only one commit with data-* only and force push it to the same branch.

@vrann vrann reopened this Sep 29, 2017
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 29, 2017

CLA assistant check
All committers have signed the CLA.

@PieterCappelle
Copy link
Contributor Author

@orlangur Done.

@orlangur orlangur changed the title Allow ID and data-* tags in TinyMCE Allow data-* tags in TinyMCE Oct 12, 2017
@orlangur orlangur changed the title Allow data-* tags in TinyMCE Allow ID and data-* tags in TinyMCE Oct 19, 2017
@orlangur
Copy link
Contributor

@PieterCappelle during testing we found out that even if we do not allow ID explicitly multiple scenarios exist where merchant is still able to add ID to an element. So, while it would be good to enforce demarcation standard, it is not that easy, we would have to change some TinyMCE internals.

Please force push current branch without last commit so that both id and data-* attributes are allowed (just like you did the fix initially).

@PieterCappelle
Copy link
Contributor Author

@orlangur No problem, changed the code.

@orlangur
Copy link
Contributor

Ah, ok, could do a revert on our side, just wanted a cleaner history :) Thanks for understanding 👍

@PieterCappelle
Copy link
Contributor Author

Should be okay, sorry for delay. :)

@orlangur orlangur added the 2.2.x label Oct 25, 2017
@orlangur orlangur added bug report hacktoberfest Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Oct 25, 2017
@orlangur
Copy link
Contributor

Ping @osrecio #11053 (comment) did this fix work for you after all? Last time there was a report from QA engineer that initial issue appears in some cases, didn't try it in action by myself yet.

@osrecio
Copy link
Member

osrecio commented Oct 30, 2017

I don't try to myself yet, I will test this afternoon. I will test with 2.2-develop branch

@osrecio
Copy link
Member

osrecio commented Oct 30, 2017

@orlangur don't works for me with id.

If I put: <img id="image-id" src="help.jpg" /> get the pop-up error with this error:

error: error in [unknown object].fireEvent(): event name: tinymceSaveContent error message: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.

If I put: <img data-test="image-data" src="help.jpg" /> works fine.

@orlangur
Copy link
Contributor

@osrecio thanks for checking!

@PieterCappelle could you please check the ID case?

@orlangur
Copy link
Contributor

@PieterCappelle could you please check if this PR actually fixes mentioned issue? I didn't have a chance to check by myself yet but there are two independent persons which still observed "Failed to execute atob on Window".

@orlangur
Copy link
Contributor

orlangur commented Dec 4, 2017

Closing for now as it seems like this change didn't fix initial issue completely, #11025 is another attempt of fix.

Thanks @PieterCappelle for your endeavour!

@orlangur orlangur closed this Dec 4, 2017
@PieterCappelle PieterCappelle deleted the fix_tinymce branch December 21, 2017 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report bugfix Progress: needs update Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants