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

[FF] Adding inline style prevents from adding white space at the end of the paragraph #692

Closed
Mgsy opened this issue Nov 27, 2017 · 9 comments · Fixed by ckeditor/ckeditor5-engine#1204
Assignees
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Mgsy
Copy link
Member

Mgsy commented Nov 27, 2017

🐞 Is this a bug report or feature request? (choose one)

  • Bug report

💻 Version of CKEditor

1.0.0-Alpha2

📋 Steps to reproduce

Scenario 1

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret at the end of the Paragraph.
  3. Activate Bold.
  4. Press Spacebar.

Scenario 2

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret at the beginning of the Paragraph.
  3. Activate Bold.
  4. Press Spacebar.

Scenario 3

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret at the beginning of the Paragraph.
  3. Activate Bold and write something.
  4. Deactivate Bold.
  5. Press Spacebar.

✅ Expected result

Space was inserted.

❎ Actual result

Scenario 1

  • Nothing happens

Scenario 2

  • The selection moves to the beginning of the document

Scenario 3

  • Nothing happens

📃 Other details that might be useful

  • The issue is not reproducible with the Bulleted/Numbered lists.
  • The issue is not reproducible with any other keys.

GIF
bug_cke5

Original issue - mozilla/notes#416
Browser: Firefox 57
OS: Windows 10, MacOS X

Edited: Added 3. Scenario.

@ma2ciek
Copy link
Contributor

ma2ciek commented Dec 5, 2017

There's also a third connected scenario:

  1. Go to http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Put the caret at the beginning of the Paragraph.
  3. Activate Bold and write something.
  4. Deactivate Bold.
  5. Press Spacebar.

@ma2ciek ma2ciek self-assigned this Dec 5, 2017
@ma2ciek
Copy link
Contributor

ma2ciek commented Dec 5, 2017

The 2. and 3. scenarios are connected with the FF <br> element, which FF adds at the end of the block element when pressing space. We filter these elements out, but something goes wrong in the MutationObserver. I'm debugging it.

@ma2ciek
Copy link
Contributor

ma2ciek commented Dec 20, 2017

It turned out, that there're two bugs, but only one was visible at the first glance.

Firefox inserts space and <br> element while typing at the end of the block elements. We're filtering out the 'bogus br mutations' in MutationObserver and passing only space. But the <br> element is connected with the passed mutation through the childList of the space textNode's parent (strong element in case 1 and 2 scenarios). The <br> element is filtered out later by the Schema, but it should have been removed before (1). The second issue, that is easily visible, is the result of removing the normal space after the inline filler by the DOM Converter and this should be fixed too at https://github.com/ckeditor/ckeditor5-engine/blob/fbfca89f6e13f770a2a768e46893b57fef08dcef/src/view/domconverter.js#L966

@ma2ciek
Copy link
Contributor

ma2ciek commented Feb 7, 2018

Update:

Manually fixing DOM and removing bogus BR fixes 1. and 3. scenarios.

The 2. is a little bit different and it's more about a bug connected to removing normal space after inline filler.

@Reinmar
Copy link
Member

Reinmar commented Feb 7, 2018

Manually fixing DOM and removing bogus BR fixes 1. and 3. scenarios.

Removing it from where? The DOM? I think that we talked about removing it from mutations?

@ma2ciek
Copy link
Contributor

ma2ciek commented Feb 8, 2018

Hmm, I've fixed the DOM mutations, so through them I was changing the original DOM. I don't know if it's easily achievable by fixing only view mutations. I'd have to somehow hack the DomConverter during conversion the Strong node's children to view.

@Reinmar
Copy link
Member

Reinmar commented Feb 8, 2018

What do you mean by "fixed the DOM mutations"? ;) Mutation is an information that we got. So, you want to say that you're reverting some mutations by changing the DOM back to its original state?

@Reinmar
Copy link
Member

Reinmar commented Feb 8, 2018

We had a short call and we agreed to first check the problem with filtering out the text node and work on the mutations later on.

These are separate issues and by fixing the filtering problem we will fix most of the issue. The br-in-mutations problem will still be something that we should work on, though.

@ma2ciek
Copy link
Contributor

ma2ciek commented Feb 14, 2018

Fix to the space is ready, but it doesn't solve all issues, I'll create a follow-up to the better <br> handling.

Reinmar added a commit to ckeditor/ckeditor5-engine that referenced this issue Mar 7, 2018
Fix: [Firefox] Added fix for typing space on the edge of inline elements. Closes ckeditor/ckeditor5#692.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
3 participants