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] Typing space after inline element boundary doesn't work #1083

Closed
Reinmar opened this issue Jun 19, 2018 · 5 comments
Closed

[FF] Typing space after inline element boundary doesn't work #1083

Reinmar opened this issue Jun 19, 2018 · 5 comments
Assignees
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2018

  1. Press bold (on),
  2. Type "a",
  3. Press bold (off),
  4. Press Space

Expected: <p><b>a</b> []</p>

Actual: <p><b>a</b>[]</p>

@Reinmar
Copy link
Member Author

Reinmar commented Jun 19, 2018

I pushed ckeditor5-typing@t/ckeditor5/1083 with a temporary fix.

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. status:confirmed labels Jun 19, 2018
@Reinmar Reinmar added this to the iteration 18 milestone Jun 19, 2018
@ma2ciek
Copy link
Contributor

ma2ciek commented Jun 19, 2018

It looks like a regression - it was fixed here: ckeditor/ckeditor5-engine#1204

@Reinmar
Copy link
Member Author

Reinmar commented Jun 19, 2018

Yes and no. The bug was there all the time. Or rather – lack of functionality. It manifested itself once we enabled support for <br>s.

The problem was in what's @pomek and @scofalik were pinging me for some time – lack of support for diffing not-only-text content:

https://github.com/ckeditor/ckeditor5-typing/blob/039f5d3e5ac4cc698208121c67e56d5642a0fe24/src/input.js#L215

@scofalik
Copy link
Contributor

scofalik commented Jun 19, 2018

@ma2ciek I think this is a little different case. This one takes place in typing. It was "hidden" earlier because we didn't convert <br />. Now we do and it messed up some algorithms in typing.

As far as the solution goes, it looks okay for now. However, I don't like this part of code

// Skip situations when common ancestor has any elements (cause they are too hard).
if ( !hasOnlyTextNodesOrBrs( modelFromDomChildren ) || !hasOnlyTextNodesOrBrs( currentModelChildren ) ) {
	return;
}

Which caused the bug. The moment we will allow more inline elements that will be rendered, the bug will manifest again.

@scofalik
Copy link
Contributor

scofalik commented Jun 19, 2018

Actually, there is a problem with indexes even now if we will have a "real" <br /> inside the text. So we need to do something to keep the indexes correct. Unfortunatelly, simply changing non-texts to a character won't work correctly cause the Firefox'es bogus br will be changed to that character and inserted.

Reinmar added a commit to ckeditor/ckeditor5-typing that referenced this issue Jun 20, 2018
Fix: Bogus `<br />` element inserted by a browser at the end of an element is now correctly handled. Closes ckeditor/ckeditor5#1083.
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
Development

No branches or pull requests

3 participants