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

Improve placeholder behaviour when ui element is present #4114

Closed
scofalik opened this issue Jul 17, 2017 · 9 comments
Closed

Improve placeholder behaviour when ui element is present #4114

scofalik opened this issue Jul 17, 2017 · 9 comments
Assignees
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

At the moment, placeholder is removed when there is a view.UIElement present in the element that has the placeholder. This is not really correct, as UIElement is not data. Most of time it will be some tiny graphical indicator like a dot, arrow, line, etc. Placeholder should be preserved in such situation.

@scofalik scofalik self-assigned this Jul 17, 2017
@Reinmar
Copy link
Member

Reinmar commented Jul 17, 2017

Should it? Where will be those UIElements rendered then? My guess is on the right hand side of the placeholder because it's rendered with ::before style.

Couldn't you hide your UIElements using CSS when content has the .ck-placeholder class?

@scofalik
Copy link
Contributor Author

scofalik commented Jul 17, 2017

Yes, of course, we have to change ::before to ::after.

Couldn't you hide you UIElements using CSS when content has the .ck-placeholder class?

No, because it is not correct.

@Reinmar
Copy link
Member

Reinmar commented Jul 17, 2017

OK, I get it now. You wouldn't be able to hide those markers anyway because the .ck-placeholder class is removed in the first place.

But once it will stop being removed if there are any markers in the view I'd propose to hide them anyway. Then, you won't need to change ::before to ::after

@scofalik
Copy link
Contributor Author

scofalik commented Jul 17, 2017

This is more difficult than I initially thought.

Things to consider:

  1. If we want to correctly display UIElements, placeholder has to be applied on ::after.
  2. But then, bogus br cannot be used as a filler element, because it moves the placeholder to the next line.
  3. We can set view.ContainerElement#getFillerOffset() to return null if it has data-placeholder attribute.
  4. Then, inline filler is created instead of bogus br.
  5. Then, after clicking a placeholder, inline filler is created and then selection is moved to the beginning of element, that is before an inline filler (which is an error).
  6. This means that we need to dig deeper, in selection rendering mechanism. We have this whole "similar selection" mechanism which prevents re-rendering selection when DOM and view selections are similar. Unfortunately this causes a bug in this case, cause we would like to reposition selection after the inline filler. I changed view.Renderer#_updateDomSelection to do it, but then new bugs appeared. And also tests started to fail.

I also tried another route with:

.ck-placeholder [data-cke-filler] {
	display: none;
}

But it also didn't seem like a good solution. UI element was rendered after bogus br and was moved to the next line. If I changed getFillerOffset() to render bogus br after UI element new errors appeared.

@scofalik
Copy link
Contributor Author

We can take easy path with what you suggested, @Reinmar, but it is not really correct. I can imagine a need to use (and display) UI element in an empty element (for example some kind of button).

@jodator
Copy link
Contributor

jodator commented Jun 24, 2019

Most of time it will be some tiny graphical indicator like a dot, arrow, line, etc. Placeholder should be preserved in such situation.

@scofalik & @Reinmar: and the first non-empty element with UI-only child is the Media Embed widget.

I'll work on this more as @oleq has pointed me this issue.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added module:view type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
@pomek pomek removed this from the backlog milestone Feb 21, 2022
@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:engine resolution:expired This issue was closed due to lack of feedback. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

6 participants