Unlink toolbar button is enabled when selection is not inside link element

[oskarwrobel]

[oleq]

The super strange thing is that is starts working as soon as the first link is created:

[oskarwrobel]

When selection is placed inside link element for the first time.

[oskarwrobel]

Maybe something related to bind() ?

[oleq]

Hard to tell without debugging. This is really odd.

[oskarwrobel]

This is because of missing initial refreshState() call in UnlinkCommand
https://github.com/ckeditor/ckeditor5-link/blob/master/src/unlinkcommand.js#L27

[oleq]

We may not need it at all https://github.com/ckeditor/ckeditor5-link/issues/52#issuecomment-294822077 ;-)

[oleq]

It turned out the issue is in ToggleAttributeCommand. Will be resolved in https://github.com/ckeditor/ckeditor5-core/issues/50.

[Reinmar]

I've just noticed that the LinkCommand doesn't base on ToggleAttributeCommand :D So no, this won't be fixed by the fix in the core.

[oleq]

@Reinmar I guess this one should fix it.

[Reinmar]

Not entirely. If you start the editor content with an image (link starts disabled) and then move the selection somewhere into a text, the link is not enabled because it's not refreshed.

[oleq]

Shouldn't https://github.com/ckeditor/ckeditor5-core/pull/81/files#diff-2647a477d39df248744db49b8051e2e8R48 handle this case?

[Reinmar]

No. LinkCommand doesn't inherit from TAC. Hence, we need the same fix in LC as you proposed in TAC.

[oleq]

You're right. Shouldn't it rather become a core feature? I mean, there will be tons of commands experiencing the same issue in the future and it's easy to forget about this case.

[Reinmar]

I guess we could have a helper (core/command/helpers) called refreshOnChange() which would add such a listener. OTOH, these are 4 LOC and we also have a ticket in the engine opened (https://github.com/ckeditor/ckeditor5-engine/issues/698) which may affect (shorten) this code. So, I'd not do anything now. It's easier to repeat ourselves for now rather than fixing broken abstraction later on. But I definitely agree that before 1.0.0 we'll need to clean this up.