Rewrite link highlighting so it adds a class to <a>

[Reinmar]

Right now, link highlighting adds another element (a <span>) which causes a lot of issues:

• Typing accented letters (Spanish-ISO) fails in a link #888,
• https://github.com/ckeditor/ckeditor5-link/issues/176,
• [Firefox] Caret is stuck at the end of a link when pressing left arrow key quickly #889 (we didn't merge Open link balloon on first click ckeditor5-link#179),
• entire IME when typing at the link beginning/end because selection is now (after Balloon panel doesn't show up after click when link is not highlighted #4823) rendered outside the <span> in which typed characters should appear.

We'll need to change this to rendering link highlight as a class of the <a> element. This can be done after ckeditor/ckeditor5-engine#1347 lands.

However, I'm afraid that it's not going to fully fix situations like #888 if, during conversion, one instance of view <a> was replaced with another instance of the same element, but with a class. In such case, we might be still hitting the unnecessary rendering issues (see https://github.com/ckeditor/ckeditor5-engine/issues/1342) which will cause the same kind of problems.

[scofalik]

Some insight how to approach this issue.

Link highlighting is based on markers. The marker is created when the selection is put inside a link. The marker range is same as link range.

In the current, simple implementation, we use default marker downcast converters. So, the marker is converted to one or more <span> elements. As mentioned, this brings some problems and has to be changed. The first idea might be to use <a> instead of <span>, but it wouldn't work because view elements created in default marker converter do not merge with other elements.

After the recent markers downcast update, the process changed a bit. The most important thing is that now, marker name is mapped to marker elements in conversion.Mapper. Marker removing is not based on ranges now. Instead, marker removing converter takes marker name and reads the elements to unwrap from conversion.Mapper. This, unfortunately, makes our case more complicated.

The solution that we need to implement here is to provide a custom downcast converter. Thankfully, in this case, we can assume some things, so the converter will be a bit simpler (for example, we know that the range will never be collapsed and we know that we are interested in a elements, and we know that a elements are probably* top-most elements).

You can base the converter on highlightText converter from /src/conversion/downcast-converters.js

First, let's tackle marker inserting converter.

Basically, DowncastDispatcher works in a way that it will fire a conversion event for each model item that is within the marker range. As I mentioned - these all will be text nodes. There are two ways to go:

1. Simply wrap mapped view range with apropriate a element. Check highlightText how to do it.
2. Map model range to view range and find a element that is "somewhere there" (most probably you could just check view range's start position's nodeAfter but I cannot guarantee that). Then, add the class and consume everything that is in data.consumable. This is a more efficient solution, although more complicated too. Here we use the fact that the a element cannot include other a elements and is probably* top-most element (so it is not broken by anything).

Now, removing converter.

This is much much more difficult. The problem is that you don't have a range there. All you have is marker name. As you could see in highlightText, in default conversion, marker elements are remembered in Mapper and that is later used when they are removed. Here, we are in much tougher spot, because:

• we don't create any new elements,
• the elements we modify are not like marker elements, so they could join with other elements, OTOH, they will join only with a element with same href so... well, they won't join a lot after all. But there are some tricky cases if you are going to remember which a you added class to.

So yeah, I don't know the correct solution for removing. A reasonable idea would be to remember the a elements which got the class, and then use those when removing. This even does not have to be done through Mapper (especially since Mapper handles marker view elements). Those elements could be stored in some data structure connected with Link plugin.

Other idea is to maybe add an id together with class. Then the a element would be treated like a marker view element by view.Writer and could be remembered by Mapper. But this may have consequences that are hard to foresee (especially when it comes to joining elements, if href would change in meantime (for example through collaboration) the <a href> view element would probably not be joined, so there would be <a> element inside <a> element)). Yeah, I'd scratch off this idea.

Another solution may be to just brute-force-a-bit. Try cleaning a bigger part of view. Maybe remember previous selection position, try to find a link element in neighbourhood and if found, remove the class. However that would have to work in two cases:

• marker is really removed, cause user moved selection,
• marker is only refreshed (so selection is not moved) because for example other plugin or collaboration changed something "in background".

We can discuss together how to tackle custom markers, this is not a trival problem.

[pjasiun]

Other idea is to maybe add an id together with class.

I was looking for this in your long post. And... I wanted to write it is a good idea, but then I started thinking about converting other link attributes and realized that we would go in the direction where a link will be an element, not an attribute anymore. I don't think this is the way to go, but maybe?

I think there is no golden bullet and we need some compromise. The idea of a custom converter which will manually find all elements and add an attribute to it sounds good to me. When it comes to removing this class we may have a view postfixer to make sure that there are no artifacts. Maybe we could keep the whole logic in a view postfixer?

Yay... it is really tricky.

[Reinmar]

realized that we would go in the direction where a link will be an element, not an attribute anymore. I don't think this is the way to go, but maybe?

That'd be like saying – text attributes are too hard so we used element for the link feature. And for the highlight too (cause we needed there the same behaviour), and so on. Suddenly, you have the DOM in the model.

We need a system solution for this. If markers are that hard to use (for more than comments and user selection) it means that they are still not complete as a feature. So, let's see what will be needed to implement link highlight and think how this can become an engine's feature

[pjasiun]

The whole idea of attributes is that they are attached to the text and if text moves, attributes moves together with this text. Splitting, merging, moving, should not matter, attributes go whenever the text goes. In this case, we want to add the class which will not move. We are considering link here, more like a block. We want to keep some of the attribute behavior (merging with other attributes), and some block behavior (easy to track). This is why this is that tricky and this is why I say that using IDs is too far in the block direction.

[Reinmar]

You're oversimplifying this.

First of all, what we want to achieve is to still have the link feature work as it does now. It's still the link feature, it still works similar to other attributes. In fact – in its base, it works like all other attributes.

Then, we say that we want to extend that behaviour. Extend by composition. We added the two step caret movement this way and it works. Then, we want to add a class to the specific element in which the selection is located. Add a behaviour to the existing feature. A pretty straightforward behaviour. It must not require a complete rewrite of a feature and dropping some design principles along that.

If the engine does not support adding a class to the currently selected link in a reasonable way, then we have pretty serious a problem. And when we have a problem you can't say that – ok, screw it, we'll make link an element in the model. I already told you that we need the same behaviour in the highlight feature. And, since this isn't some theoretical feature, there's a great chance that other developers will try to achieve similar things.

Anyway, I can't put this differently – by saying that we need to rewrite a link just because we need to add a class to it, you're saying that the engine is broken.

PS. Another feature which from the user perspective acts this way – comments.

[Reinmar]

This is why this is that tricky and this is why I say that using IDs is too far in the block direction.

Since I'm not sure I understand @scofalik's idea of adding ids I hope I just misunderstood what you wrote ;)

Anyway, looking from "afar", I'd imagine that this feature (link highlight) can be implemented in such a way:

1. you listen to model's selection change (or document change in general)
2. when selection is in a link (in the model or in the view – it actually can be the latter), you just get the view's link element and add a class to it
3. you store this view element instance locally
4. when document changes again, you check the status of things
   • you ask the view about all elements which might've inherited anything from the stored element
     (e.g. when <a> was split); alternatively, we can scan part of the view tree in a search for <a> elements;
   • you remove the class from elements which shouldn't have it and add to the element which should have it

That's how you'd approach this initially. Actually, that's what Olek tried on the hackathon. It's simply – the most straightforward approach.

Now, the tricky part, I understand, is in quickly finding all the <a> elements which need to be checked. Let's maybe think how we can make this possible in the easiest and fastest possible way.

E.g. if we had a fast implementation of getElementByTagName() (it can be based on a constantly maintained map of these elements; like browsers do natively), it would be completely sufficient up to documents with hundreds of links.

Or, maybe we could go further – have a quick implementation of getElementByClassName() (again, based on a map) – that would be even more performant and super useful for other developers too. Look, the view becomes the DOM!

Or, maybe we can have just one, special attribute (or element's property) which would allow tracking them and their clones? That would be even better. But I guess that this is what @scofalik meant and it causes problems when squashing elements (how did we call vertical merges?). Then, maybe we should improve that, so you can control it or introduce ids and uids?

I don't know. Let's just think on what we miss in the engine because we clearly miss something.

[scofalik]

We are not making links into elements. I agree with @Reinmar on this one. From the beginning, the concept of link highlight didn't seem difficult, and it should not be. If it is and if we have problems with attributes, then that means that the architecture went wrong at some point.

When I write about attribute element's id, I mean view.AttributeElement#id property, not DOM id attribute. Just to be clear. This is the property that is introduced so that: attribute element's don't squash and are more easily trackable.

Or, maybe we can have just one, special attribute (or element's property) which would allow tracking them and their clones?

That's exactly it. But squashing makes it difficult, especially if multiple elements had different ids. Then, suddenly, id is a Set, not a, say, String. Of course, everything is doable, but it is hard and will take time and possibly introduce regressions. I'd rather avoid it if we can, at least for now.

Since I'm not sure I understand @scofalik's idea of adding ids I hope I just misunderstood what you wrote ;)

I think that what @pjasiun meant is that adding view.AttributeElement#id to already existing attribute elements is going too far, and I agree.

Back to the original problem.

Maybe I tackled it from the wrong angle. When @Reinmar writes down the "initial conceptual solution" like that, it seems easy and obvious way to go with it. Why it was not made like that from the beginning? Why were markers used? I've gone with markers solution cause I assumed that we need markers here. If we don't use markers, we can go in completely different direction. The question is what problems will we meet there. Personally, I think that using markers for such thing is a bit smelly, so I am open to discovering other routes. If you want to discuss it, let's go.

PS. OTOH, if past is any indicator, messing around straight inside the view always goes bad, sooner or later.

On another topic. @Reinmar, you wrote that maybe markers are not complete (well designed) feature if we have such problems with them. But please, keep in mind that as much as we wanted to be able to write custom markers converters, I think that we always thought about adding elements in the view. This is, in my opinion, an exceptional case, where marker changes the elements that already are in the view. It is a little bit like List feature. It is complicated. We changed some stuff there multiple times. We don't expect an average user to write a feature like List. It's doable but it is an exception case for model element conversion.