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

Placing caret on the beginning of the link on the line start reinserts whole link DOM element #4354

Closed
f1ames opened this issue Jun 7, 2018 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1424

Comments

@f1ames
Copy link
Contributor

f1ames commented Jun 7, 2018

The link highlight is a ck-link_selected class added to a link element. It's an attribute so it should be added by the renderer._updateAttrs() method. This happens in most cases, for example, when moving caret like:

1. <p><a href="123">Foo</a> B{}ar</p>
2. <p><a href="123">F{}oo</a> Bar</p>
3. <p><a href="123">Fo{}o</a> Bar</p>

renderer adds highlight class via renderer._updateAttrs() method (as a element was marked to have its attributes updated) and then the renderer._updateChildren() method is run. Since nothing has changed, DOM structure is not modified.

When the caret is placed on the beginning of a link (within text) like:

1. <p>F{}oo <a href="123">Bar</a></p>
2. <p>Foo {}<a href="123">Bar</a></p>

attributes are not modified (since selection is still outside of the link), or like:

1. <p>Foo <a href="123">Ba{}r</a></p>
2. <p>Foo {}<a href="123">Ba{}r</a></p>

highlight class is simply removed in renderer._updateAttrs() method and no DOM rendering takes place.


The situation is quite different when placing selection on the beginning of the link which is a first element in the paragraph. There are two cases.

Moving selection from regular text to the link:

1. <p><a href="123">Foo</a> B{}ar</p>
2. <p><a href="123">{}Foo</a> Bar</p>
  1. No elements are marked to have its attributes updated (renderer.markedAttributes is empty).
  2. The p and a elements are marked to be updated (renderer.markedChildren)1.
  3. Children of the p element are updated in renderer._updateChildren(). Since a view element is different, its DOM representation gets rerendered (old a is replaced with new a.ck-link_selected).
  4. Now renderer._updateChildren() is executed for the old a view element. Since it has no parent and cannot be mapped to the DOM, nothing changes.

So here the fact that link is highlighted is simply caused by the fact that a element was replaced, not because its attributes were updated (as it should happen IMHO).

Moving selection within the link:

1. <p><a href="123">Fo{}o</a> Bar</p>
2. <p><a href="123">{}Foo</a> Bar</p>

Here situation is similar but with additional renderer._updateAttrs() call.

  1. The a elements is marked to have its attributes updated (renderer.markedAttributes)1.
  2. The p and a elements are marked to be updated (renderer.markedChildren)1.
  3. The attributes of a are updated in the renderer._updateAttrs() method. Highlight ck-link_selected class is removed.
  4. Children of the p element are updated in renderer._updateChildren(). Since a view element is different, its DOM representation gets rerendered (old a is replaced with new a.ck-link_selected).
  5. Now renderer._updateChildren() is executed for the old a view element. Since it has no parent and cannot be mapped to the DOM, nothing changes.

In this case, renderer._updateAttrs() removes highlight class and then it's added because whole a element is rerenderd.

1. When a (view element) is being marked to sync it has a parent element but when rednerer.render() is called, its parent is already empty (which means it was removed from view structure and replace with another element).


I think in all the cases above, DOM a element should not be reinserted, only its attributes updated. Still I'm not sure if we should mark it as a bug or enhancement.

@Reinmar
Copy link
Member

Reinmar commented Jun 7, 2018

Definitely. We should only touch this element's attributes.

It would be good to understand why p's children are marked to be rendered. Most likely, unfortunately, due to selection's conversion. That may be again the same story that the selection is converted next to the existing <a> element so this happens for a while:

<p><a>[]</a><a>xxx</a></p>

And then merge happens... to the left side. So we lose the reference to the original <a> element (the one on the rigth hand side). And, at the same time, we mark <p> children to be re-rendered. There's a ticket in which I described this mechanism. Do you remember which was it?

Now... your PR should theoretically fix this. Why doesn't it do this? Doesn't it find the new <a> a replacement of the old <a>?

@f1ames
Copy link
Contributor Author

f1ames commented Jun 8, 2018

And, at the same time, we mark <p> children to be re-rendered. There's a ticket in which I described this mechanism. Do you remember which was it?

I think you mean https://github.com/ckeditor/ckeditor5-engine/issues/1342?

Now... your PR should theoretically fix this. Why doesn't it do this? Doesn't it find the new a replacement of the old ?

It does, but due to the fact that it rebinds mappings (so the old a view element is removed from mappings), renderer._updateAttrs which used this old a element to update attributes throws an error (becuase there is no corresponding DOM element). It looks like more generic problem with updating attributes of elements which was replaced in a view structure. I'm looking for some reasonable solution for this case now.

Reinmar referenced this issue in ckeditor/ckeditor5-engine Jun 18, 2018
Fix: Renderer should avoid doing unnecessary DOM structure changes. Ensuring that the DOM gets updated less frequently fixes many issues with text composition. Closes #1417. Closes #1409. Closes #1349. Closes #1334. Closes #898. Closes ckeditor/ckeditor5-typing#129. Closes ckeditor/ckeditor5-typing#89. Closes #1427.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants