Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Bogus <br /> handling during typing #156

Merged
merged 2 commits into from
Jun 20, 2018
Merged

Bogus <br /> handling during typing #156

merged 2 commits into from
Jun 20, 2018

Conversation

scofalik
Copy link
Contributor

Suggested merge commit message (convention)

Fix: Bogus <br /> element inserted by a browser at the end of an element is now correctly handled. Closes ckeditor/ckeditor5#1083.


Additional information

This is not a complete solution. I don't like two things:

  • Inserting @ in place of non-text elements. I tested it manually and it looks like everything works well but I am a little skeptical because a user may type @ by themselves and I am afraid it may mess up the diff. Still, I tried putting <br /> between typed @s and it worked 🤷‍♂️ .
  • Only <softBreak> "inline" element is now supported. I wanted to do it in a future-proof way but I don't know how you can differentiate that given elements are "inline" elements (like <softBreak>). Basically, when we will introduce a new inline element, like smileys or maybe placeholders, we again will have a problem in isSafeForTextMutation function. Maybe we could introduce isInline flag in SchemaItemDefinition?

@coveralls
Copy link

coveralls commented Jun 20, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling fb4e3c1 on t/ckeditor5/1083 into 039f5d3 on master.

@Reinmar
Copy link
Member

Reinmar commented Jun 20, 2018

OK, I tested it quite thoroughly and will merge it. However, let's have a confirmation – @pomek, could you check it too? Try being creative :D And remember to check it on all browsers.

@pomek
Copy link
Member

pomek commented Jun 20, 2018

I'll do all the best what I can in order to check this out :D

@Reinmar Reinmar merged commit 22abdff into master Jun 20, 2018
@Reinmar Reinmar deleted the t/ckeditor5/1083 branch June 20, 2018 13:05
@Reinmar
Copy link
Member

Reinmar commented Jun 20, 2018

I reported ckeditor/ckeditor5#1096 as a followup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants