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

Race condition between the link UI and the contextual toolbar #852

Closed
oleq opened this issue Feb 15, 2018 · 10 comments
Closed

Race condition between the link UI and the contextual toolbar #852

oleq opened this issue Feb 15, 2018 · 10 comments
Labels
package:link package:ui resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@oleq
Copy link
Member

oleq commented Feb 15, 2018

🐞 Is this a bug report or feature request? (choose one)

  • Bug report

πŸ’» Version of CKEditor

master

πŸ“‹ Steps to reproduce

  1. Visit http://localhost:8125/ckeditor5-editor-balloon/tests/manual/ballooneditor.html
  2. Follow the screencast
    kapture 2018-02-15 at 14 30 37

βœ… Expected result

Which one should prevail? We need to understand which one has a greater priority in terms of UX.

❎ Actual result

UIs show up at random upon click.

πŸ“ƒ Other details that might be useful

Related #845

cc @Reinmar @dkonopka

@oleq oleq added type:bug This issue reports a buggy (incorrect) behavior. candidate:1.0.0 package:link package:ui labels Feb 15, 2018
@oleq oleq added this to the iteration 14 milestone Feb 15, 2018
@scofalik
Copy link
Contributor

For non-collapsed selection I think that toolbar should be shown. For collapsed it is discussible, but I also think that toolbar should be shown and the link ui will be still accessible through link icon.

@oleq
Copy link
Member Author

oleq commented Feb 15, 2018

The problem is that when a new link is created it gets selected as a whole. We'd need to drop this behavior.

@Reinmar
Copy link
Member

Reinmar commented Feb 16, 2018

The behaviour that link gets selected after it's created, theoretically, allows us to override link's text after we do https://github.com/ckeditor/ckeditor5-link/issues/73. So, I think that in such a case it should stay.

Alternatively, we could do the same thing which GDocs does, so add a "link text" input to the link balloon. And you can notice that GDocs makes a collapsed selection at the end of the newly inserted link.

BTW, this is actually a DUP of https://github.com/ckeditor/ckeditor5-link/issues/152. But I created #152 in non-general enough way, so I'll merge that one into this issue.

@jodator
Copy link
Contributor

jodator commented Feb 16, 2018

So for me it looks like the behavior is due to "double click" behavior that selects texts.
In other words:

  • first click: shows link editing balloon
  • second click (done within doubleclick timeout): will select whole word and thus will display the ballon toolbar.

@Reinmar
Copy link
Member

Reinmar commented Feb 16, 2018

My comments from ckeditor/ckeditor5-link#152:

I actually lost confidence here when the link balloon pops up for non-collapsed selections.

PS. There's also an interesting behaviour in Letters which might be caused by the same issue:

dec-18-2017 16-09-02

And they are all related to https://github.com/ckeditor/ckeditor5-ui/issues/354.

@jodator
Copy link
Contributor

jodator commented Feb 16, 2018

Which one should prevail? We need to understand which one has a greater priority in terms of UX.

The problem in my opinion here is that both are required just in different contexts - as it is now...

When I click the link - the UI for changing/removing link looks OK.

When I select part of the text with link I might want to make something with that selection (like bold).

One solution is to add timeout and try to "detect" double click - but such solution would probably make normal single-click on link look sluggish :(.

@Reinmar
Copy link
Member

Reinmar commented Mar 27, 2018

This is annoying but not super common and we have more important things to fix now and there's no easy solution here, so I removed the milestone.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:link package:ui resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

7 participants