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

Balloon panel doesn't show up after click when link is not highlighted #4823

Closed
oskarwrobel opened this issue Mar 8, 2018 · 2 comments · Fixed by ckeditor/ckeditor5-link#189
Assignees
Labels
package:link type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oskarwrobel
Copy link
Contributor

mar-08-2018 13-35-21

@szymonkups szymonkups self-assigned this Mar 8, 2018
@szymonkups
Copy link
Contributor

I've tested this a little and it looks like this is because highlight's <span> element is created on top of <a> element. The failing flow is like this:

  1. User clicks on a link,
  2. selectionchange event is fired before click event, it triggers changes on the model,
  3. Postfixer is adding marker over link,
  4. Marker is rendered as <span> over <a> element and Renderer is creating brand new <a> in the DOM,
  5. No click event since old <a> is removed from DOM.

We will fix this by adding highlight with higher priority than link one, so the <span> will be rendered inside <a> element.

@Reinmar
Copy link
Member

Reinmar commented Mar 13, 2018

The PR proposed by @szymonkups in ckeditor/ckeditor5-link#179 was a workaround which changed how the highlight is rendered. From:

<span class=highlight><a href=#>xxx</a></span>

To:

<a href=#><span class=highlight>xxx</span></a>

It fixes this issue in Chrome, but:

  • it's still broken in Firefox and Safari,
  • new issues appear.

However, most importantly, it's still wrong – the highlight must not be rendered as a separate span. It should be a class on the original <a> element. Toggling a class will be much safer than changing the DOM structure.

Unfortunately, switching to a class-based highlight is blocked now by a much bigger issue. Therefore, we won't manage to fix this issue before releasing beta.1.

So, I think that we should still go with the highlight on and live with this bug for a while. It definitely needs to be fixed before the first stable release and it's definitely quite critical. However, we also need to be able to see how the link highlight feature behaves – it's better not to add it like that in a final release.

@oleq oleq self-assigned this Apr 4, 2018
scofalik referenced this issue in ckeditor/ckeditor5-link Apr 5, 2018
Fix: The selected link should be highlighted using the class instead of a marker. Closes #180. Closes #176. Closes ckeditor/ckeditor5#888.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-link Oct 9, 2019
@mlewand mlewand added this to the iteration 15 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:link labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:link type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
5 participants