T/ckeditor5 typing/101 Multiple spaces fix #1083

scofalik · 2017-08-18T11:03:26Z

Suggested merge commit message (convention)

Fix: Multiple spaces in an empty paragraph are now allowed. Closes https://github.com/ckeditor/ckeditor5-typing/issues/101.

Additional info

The whole algorithm got simplified and fixed and a few other edge cases has been also fixed.

…sion.

… has changed.

Reinmar · 2017-08-18T11:35:33Z

Why did you expose getTouchingTextNode as a util? Is it necessary?

…

Sent from my iPhone

On 18 Aug 2017, at 13:03, Szymon Cofalik ***@***.***> wrote: Suggested merge commit message (convention) Fix: Multiple spaces in an empty paragraph are now allowed. Closes ckeditor/ckeditor5-typing#101. You can view, comment on, or merge this pull request online at: #1083 Commit Summary Changed: Introduced `view.utils` and `view.utils.getTouchingTextNode`. Fixed: Fixed handling spaces in view text nodes in view-to-dom conversion. Changed: Renderer should re-render also next text node if a text node has changed. File Changes M src/view/domconverter.js (114) M src/view/renderer.js (17) A src/view/utils.js (41) M tests/view/domconverter/view-to-dom.js (79) M tests/view/renderer.js (21) A tests/view/utils.js (58) Patch Links: https://github.com/ckeditor/ckeditor5-engine/pull/1083.patch https://github.com/ckeditor/ckeditor5-engine/pull/1083.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

scofalik · 2017-08-18T12:06:26Z

It is used in domconverter.js and renderer.js and I couldn't find any good place where to fit it. I think that with time more functions might join there. For example I've used two times already a function that takes a view position and moves it forward after all ui elements.

If I were to put it in some existing place I'd probably do it as an exported function treewalker.js or maybe in writer.js, although the function does not change the view in any way...

Reinmar · 2017-08-21T08:53:07Z

src/view/renderer.js

@@ -420,6 +425,12 @@ export default class Renderer {

 		if ( actualText != expectedText ) {
 			domText.data = expectedText;
+
+			const nextTouchingTextNode = getTouchingTextNode( viewText, true );


Missing comment why we need to do that.

Reinmar · 2017-08-21T09:08:22Z

src/view/utils.js

+/**
+ * Contains utility functions for working on view.
+ *
+ * @module engine/view/utils


Let's rename this to walker-utils since the utils that come to our mind are Walker related. We also considered putting this to the walker's module but we don't want to make it too big (hence, a new module).

Reinmar · 2017-08-21T09:16:50Z

src/view/utils.js

+ * in the same container element. If there is no such sibling, `null` is returned.
+ *
+ * @param {module:engine/view/text~Text} node Reference node.
+ * @param {Boolean} getNext If `true` next touching sibling will be returned. If `false` previous touching sibling will be returned.


It'd be good to find it a better name... but I can't :D. lookRight?

Also, this is an optional param. Should be marked with []. Maybe this will clarify its meaning (typically, boolean flags are optional).

As for this flag being optional – it's always passed in the tests: https://github.com/ckeditor/ckeditor5-engine/pull/1083/files#diff-41de3c28cf4c9630dfaec4e35850f0faR38. This should be changed also there.

Reinmar · 2017-08-21T10:18:11Z

src/view/renderer.js

@@ -407,6 +406,12 @@ export default class Renderer {
 	 */
 	_updateText( viewText, options ) {
 		const domText = this.domConverter.findCorrespondingDomText( viewText );
+
+		// If this is a new text node and it is not in DOM, it will be created and handled in `_updateChildren`.
+		if ( !domText ) {


This looks very suspicious. How did this function work without this condition so far? Why is it needed here now?

It worked because there was a condition in view.Renderer#render method.

So why doesn't that condition save us from adding one here?

I don't understand. Before there was a condition in render method in the loop where _updateText is called. I removed this condition because I wanted _updateText to be fired more often. But this caused a situation where _upadateText was fired on text nodes that weren't yet converted to DOM. That's why I needed to add the statement in _updateText.

Aaand it turned out we can't make this change ;> We'll also need to make a step back and remove the fix for subsequent text nodes updating. It seems that we may not be able to finalise it now because of how the text nodes are mapped.

We can neither update all text nodes before children are updated because changes in children might've changed the indexes of text nodes (so updating text nodes would fail). Changing the order to 1) update children, 2) update text nodes doesn't help too because in the following case, the whole paragraph is updated first and then text nodes are not:

x b

Type space after "x".

Paragraph's children get updated (because for some reason the browser mutated text nodes too much when typing the space).

The "x " text node is up to date when it comes to updating text nodes, so we don't mark " b" to be updated.

@scofalik will you report a ticket for this case and the one you mentioned in #1083 (comment)? And of course, please revert changes which we can't do now.

https://github.com/ckeditor/ckeditor5-engine/issues/1093

Reinmar · 2017-08-21T10:18:29Z

src/view/utils.js

+	for ( const value of treeWalker ) {
+		if ( value.item.is( 'containerElement' ) ) {
+			// ViewContainerElement is found on a way to next ViewText node, so given `node` was first/last
+			// text node in it's container element.


Reinmar · 2017-08-21T10:33:10Z

tests/view/domconverter/view-to-dom.js

-			test( [ 'x    ', '   y' ], 'x_ _ _ _y' );
+			test( [ 'x    ', '   y' ], 'x _ _ _ y' );
+
+			// "Non-empty" + "empty" text nodes.


What about mirrored cases with "empty" + "non-empty"?

Reinmar · 2017-08-21T10:41:29Z

All works fine, but a couple of things need to be improved in the code.

scofalik · 2017-08-21T14:39:34Z

Unfortunately there is one more bug because text nodes are not refreshed when a node is removed or inserted. This is handled in _updateChildren not in _updateText.

For example,  x becomes x x.

EDIT:
Looking at _updateChildren and how it works, it seems a bit tricky how to trigger refreshing text nodes in a nice way. The algorithm bases on what domConverter.viewChildrenToDom and diffs it with current DOM, then operates only on DOM. The truth is that viewChildrenToDom returns wrong value at the moment, because it will not refresh text node if it was in already mapped AttributeElement. It saves us some processing time, but produces wrong results. I think that the only sane way is to fix viewChildrenToDom so it will refresh all text nodes inside given view element deeply, but this may have some impact on rendering time.

Reinmar · 2017-08-21T15:15:57Z

Let's not enlarge this PR with this case. Refreshing sibling text nodes was already something that we added here.

…w->dom conversion.

…Moved `getTouchingTextNode` back to `view.DomConverter`.

scofalik · 2017-08-22T11:07:12Z

Ready for review. Added #1093

scofalik added 3 commits August 18, 2017 12:01

Changed: Introduced view.utils and view.utils.getTouchingTextNode.

46084a2

Fixed: Fixed handling spaces in view text nodes in view-to-dom conver…

91dbb6a

…sion.

Changed: Renderer should re-render also next text node if a text node…

99d5f24

… has changed.

scofalik changed the title ~~T/ckeditor5 typing/101~~ T/ckeditor5 typing/101 Multiple spaces fix Aug 18, 2017

Reinmar reviewed Aug 21, 2017

View reviewed changes

Merge branch 'master' into t/ckeditor5-typing/101

e29624b

Reinmar reviewed Aug 21, 2017

View reviewed changes

Docs and minor regexp correction.

4ca5095

Reinmar reviewed Aug 21, 2017

View reviewed changes

Reinmar added the status:review- label Aug 21, 2017

Changed: minor fixes and several new tests for handling spaces in vie…

dbb7b6f

…w->dom conversion.

scofalik removed the status:review- label Aug 22, 2017

Merge branch 'master' into t/ckeditor5-typing/101

decc702

Reinmar added the status:review- label Aug 22, 2017

Changed: Reverted refreshing touching text nodes in view.Renderer. …

4aaaf7b

…Moved `getTouchingTextNode` back to `view.DomConverter`.

scofalik removed the status:review- label Aug 22, 2017

Merge branch 'master' into t/ckeditor5-typing/101

9515717

Reinmar merged commit 9ca61d5 into master Aug 22, 2017

Reinmar deleted the t/ckeditor5-typing/101 branch August 22, 2017 11:32

scofalik mentioned this pull request Oct 9, 2019

Renderer does not re-render spaces in text nodes correctly ckeditor/ckeditor5#4156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/ckeditor5 typing/101 Multiple spaces fix #1083

T/ckeditor5 typing/101 Multiple spaces fix #1083

scofalik commented Aug 18, 2017 •

edited

Loading

Reinmar commented Aug 18, 2017 via email

scofalik commented Aug 18, 2017

Reinmar Aug 21, 2017

Reinmar Aug 21, 2017

Reinmar Aug 21, 2017

Reinmar Aug 21, 2017

Reinmar Aug 21, 2017 •

edited

Loading

scofalik Aug 21, 2017 •

edited

Loading

Reinmar Aug 21, 2017

scofalik Aug 22, 2017 •

edited

Loading

Reinmar Aug 22, 2017

Reinmar Aug 22, 2017

scofalik Aug 22, 2017

Reinmar Aug 21, 2017

Reinmar Aug 21, 2017

Reinmar commented Aug 21, 2017

scofalik commented Aug 21, 2017 •

edited

Loading

Reinmar commented Aug 21, 2017

scofalik commented Aug 22, 2017 •

edited

Loading

T/ckeditor5 typing/101 Multiple spaces fix #1083

T/ckeditor5 typing/101 Multiple spaces fix #1083

Conversation

scofalik commented Aug 18, 2017 • edited Loading

Suggested merge commit message (convention)

Additional info

Reinmar commented Aug 18, 2017 via email

scofalik commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

scofalik Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Aug 21, 2017

scofalik commented Aug 21, 2017 • edited Loading

Reinmar commented Aug 21, 2017

scofalik commented Aug 22, 2017 • edited Loading

scofalik commented Aug 18, 2017 •

edited

Loading

Reinmar Aug 21, 2017 •

edited

Loading

scofalik Aug 21, 2017 •

edited

Loading

scofalik Aug 22, 2017 •

edited

Loading

scofalik commented Aug 21, 2017 •

edited

Loading

scofalik commented Aug 22, 2017 •

edited

Loading