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

TINY-10892: Fix deleting from P with IMG into LI would create unwanted font-styles on Chrome #9638

Closed
wants to merge 11 commits into from

Conversation

hamza0867
Copy link
Contributor

@hamza0867 hamza0867 commented May 12, 2024

Related Ticket: TINY-10892

Description of Changes:

  • Fix deleting from P with IMG into LI would create unwanted font-styles on Chrome

Pre-checks:

  • Changelog entry added
  • Tests have been added (if applicable)
  • Branch prefixed with feature/, hotfix/ or spike/

Review:

  • Milestone set
  • Docs ticket created (if applicable)

GitHub issues (if applicable):

@hamza0867 hamza0867 added this to the 7.1.1 milestone May 12, 2024
@hamza0867 hamza0867 requested a review from a team as a code owner May 12, 2024 21:53
Copy link
Member

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

I'll review it when the unnecessary format changes are reversed.

modules/tinymce/src/core/main/ts/util/Quirks.ts Outdated Show resolved Hide resolved
.changes/unreleased/tinymce-TINY-10892-2024-05-12.yaml Outdated Show resolved Hide resolved
@hamza0867 hamza0867 requested review from TheSpyder, a team, lostkeys, lorenzo-pomili and ztomaszyk and removed request for a team and lostkeys May 13, 2024 08:10
Copy link
Member

@spocke spocke left a comment

Choose a reason for hiding this comment

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

This fix adds some weird undo levels. I really should have looked into this earlier since I think we just need to add a ElementType.isListItem here: https://github.com/tinymce/tinymce/blob/main/modules/tinymce/src/core/main/ts/delete/BlockMergeBoundary.ts#L60 that is whats preventing the the merge blocks code from working with lists. There might be issues with merging list items into list items since some of that is overrided in the lists plugin so that you would outdent the list instead. But a complete override is better than trying to let the browser do it's thing, have a bunch of tracking states on that easier to test and easier to predict the behavior.

modules/tinymce/src/core/main/ts/util/Quirks.ts Outdated Show resolved Hide resolved
.changes/unreleased/tinymce-TINY-10892-2024-05-12.yaml Outdated Show resolved Hide resolved
modules/tinymce/src/core/main/ts/util/Quirks.ts Outdated Show resolved Hide resolved
modules/tinymce/src/core/main/ts/util/Quirks.ts Outdated Show resolved Hide resolved
modules/tinymce/src/core/main/ts/util/Quirks.ts Outdated Show resolved Hide resolved
@hamza0867 hamza0867 requested a review from spocke May 13, 2024 19:35
@hamza0867 hamza0867 changed the base branch from main to release/7 May 14, 2024 15:44
@hamza0867 hamza0867 requested a review from noxuhax as a code owner May 14, 2024 15:44
@hamza0867 hamza0867 changed the base branch from release/7 to main May 14, 2024 15:44
Copy link
Member

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

Well! That was quite a lesson in learning what the editor is capable of before building something that already exists 😂

I'm not sure I knew it existed either, to be fair.

Copy link
Member

Choose a reason for hiding this comment

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

It's no longer really a chrome test, nor it is a quirks test 🤔

I'm happy to keep the test, just rename it (and remember the describe call includes the name).

Copy link
Member

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

I just saw #9648. We don't need two PRs for the same fix, as release/7 will be merged to main after release.

@hamza0867
Copy link
Contributor Author

Closing this PR as #9648 is the PR that should have existed to begin with.

@hamza0867 hamza0867 closed this May 15, 2024
@hamza0867 hamza0867 deleted the feature/TINY-10892 branch May 15, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants