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

The editor placeholder should disappear when typing, not when focusing the editor #8689

Closed
oleq opened this issue Dec 16, 2020 · 12 comments · Fixed by #8867
Closed

The editor placeholder should disappear when typing, not when focusing the editor #8689

oleq opened this issue Dec 16, 2020 · 12 comments · Fixed by #8867
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:engine package:heading package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@oleq
Copy link
Member

oleq commented Dec 16, 2020

📝 Provide a description of the improvement

The idea is that the editor placeholder (editing root, image caption) should act like the native DOM input placeholder. However, its text disappears as soon as the user focuses the editable (or image caption) while in the native implementation, it should stay until the first character is typed.

Actual

Expected

屏幕录制2020-11-18 11 53 44

📃 Other details

This issue has been spotted by the community and addressed in a pull request. Although the solution makes sense, I expressed some concerns regarding the rendering of image caption placeholders. The discussion must be resolved before the PR can be accepted.


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@oleq oleq added type:improvement This issue reports a possible enhancement of an existing feature. package:engine domain:ui/ux This issue reports a problem related to UI or UX. squad:core Issue to be handled by the Core team. labels Dec 16, 2020
@mlewand mlewand added this to the nice-to-have milestone Jan 11, 2021
@maxbarnas maxbarnas modified the milestones: nice-to-have, iteration 40 Jan 19, 2021
@maxbarnas maxbarnas self-assigned this Jan 19, 2021
@maxbarnas
Copy link
Contributor

maxbarnas commented Jan 20, 2021

Just noticed that an editor without textual content, but with e.g. Bold attribute, hides the placeholder. Seems like it's tripping the isEmptyish condition. Should we check that the textual content is empty instead?

@Reinmar
Copy link
Member

Reinmar commented Jan 20, 2021

I'm not sure I understand the question.

How it works currently is that when you focus the editor, the placeholder disappears. That's most likely somehow connected with some isFocused flag.

You can only have bold on an empty content when the editor is focused so that may be hiding the placeholder. This is – the focus, not the existence of the bold attribute.

Could you give more details on what you meant?

@maxbarnas
Copy link
Contributor

maxbarnas commented Jan 20, 2021

I've been referring to this draft PRs code: https://github.com/ckeditor/ckeditor5/pull/8867/files#diff-4691af5a35179ee901044c054fabd91c8baffb422338eaaba2a9a57121869541R155-R156

The behavior this PR introduces looks as below:

placeholder.bold.mov

From what I was able to check, the isEmptyish flag in the https://github.com/ckeditor/ckeditor5/pull/8867/files#diff-4691af5a35179ee901044c054fabd91c8baffb422338eaaba2a9a57121869541R155-R156 looks for children that are not a uiElement, but by looking in the CKE dev-tools I can see a <strong> element inside the view when Bold is enabled, which, I suppose, is the reason the flag isEmptyish is false and hides the placeholder when it shouldn't.

@Reinmar
Copy link
Member

Reinmar commented Jan 20, 2021

Oh, I didn't know it works on the view. I thought it works on the model. It makes sense what you described.

I don't want to analyze the logic there, but attribute elements themselves are not content indeed. From the nodes that can be treated as content I'd list from the top of my head:

  • text,
  • EmptyElement,
  • RawElement,
  • uh... and widgetized container elements

The tricky bit is the container elements – empty <p> is not a content I guess. Empty <h1>... IDK.

What's worse – what if you have a <div> that's a placeholder for some media. It's empty for now, but it's a content already (it's styled to not look like empty). In the model it'd be marked as an object element (hence - a content).

So, there are two options:

  • map container elements to the model and check what they represent there (dunno if doable),
  • assume that the only case with an empty container element being "content" that we support is a widget and hardcode a check for that,
  • introduce a new custom property "isContent" to be a solution for this edge case and perhaps some other problems we didn't foresee.

I'm afraid the first two options are not doable (mapping to the model) or would be super ugly (hardcoding a widget check is ugly as it crosses the engine->widget package boundary). So that leaves us with the 3rd option.

I'd double-check the idea with @niegowski.

@Reinmar
Copy link
Member

Reinmar commented Jan 20, 2021

PS. some more context – we've been changing our mind regarding whether the placeholder extension scans the model or the view. That's why I thought it checks the model.

Checking the model is easier but for some reason we didn't go with this. I don't recall now why. My guess is that it was wrong to have a view feature crossing the boundary to the model. But there might be more to that.

@niegowski
Copy link
Contributor

The tricky bit is the container elements – empty <p> is not a content I guess. Empty <h1>... IDK.

I guess we should first decide what is the definition of an "empty" editor from the user's point of view so whether some styling change is considered an empty editor or not. The placeholder purpose is to indicate the "imputable" area, but if the user already changed block type from paragraph to heading or set text formatting to bold or anything else... I would assume that it's content from the placeholder perspective.

@maxbarnas
Copy link
Contributor

maxbarnas commented Jan 20, 2021

The placeholder purpose is to indicate the "imputable" area, but if the user already changed block type from paragraph to heading or set text formatting to bold or anything else... I would assume that it's content from the placeholder perspective.

Was thinking about the same thing. Instead of trying to figure out which element is/can be empty, we can just assume that for the purpose of placeholder empty means "single paragraph without text".

The Title plugin and Image Caption both behave in a similar manner. The only difference is that Image Caption has hideOnFocus = true by default, which makes the problem unnoticeable.

@oleq
Copy link
Member Author

oleq commented Jan 21, 2021

I didn't investigate the topic, just scanned the discussion, and here's what I think:

The behavior this PR introduces looks as below:

The PR does not "introduce this behavior" because this is not a regression. It's a minor (IMO) flaw in an improvement over what we used to have. It does not block this PR and it is a perfect candidate for a follow-up ;-)

@maxbarnas
Copy link
Contributor

The PR does not "introduce this behavior" because this is not a regression. It's a minor (IMO) flaw in an improvement over what we used to have. It does not block this PR and it is a perfect candidate for a follow-up ;-)

Cool beans! Fine with me :)

@Reinmar
Copy link
Member

Reinmar commented Jan 22, 2021

  • introduce a new custom property "isContent" to be a solution for this edge case and perhaps some other problems we didn't foresee.

That'd help for #8880. If not in this ticket (IDK if it's in its scope) we could extract it and work on it later.

The idea is that toWidget() would set that custom property, meaning that you don't have to be aware of whether UIElement or RawElement works for you.

Although, I think that it'd render the RawElement idea unnecessary :D The usecase for it was to differentiate content and not a content. Since we only use RawElement inside widgetized containers, it'd be also replaced back with UIElement and no harm would be done. I don't recall whether we had other reasons to introduce RawElement but perhaps it was a mistake :(

@Reinmar
Copy link
Member

Reinmar commented Jan 22, 2021

The placeholder purpose is to indicate the "imputable" area, but if the user already changed block type from paragraph to heading or set text formatting to bold or anything else... I would assume that it's content from the placeholder perspective.

There's one case, though, that makes this more complicated. There were requests to allow having an empty <h1> as the default content of the editor and still displaying the placeholder. IDK if we handle it right now, though.

The other thing you need to remember is the Title plugin that renders two placeholders: https://ckeditor.com/docs/ckeditor5/latest/features/title.html

I don't have a strong opinion over what should happen if someone applied bold to an empty editor or changed the default block, but just wanted to let you know about some more things to consider.

@maxbarnas
Copy link
Contributor

The other thing you need to remember is the Title plugin that renders two placeholders: ckeditor.com/docs/ckeditor5/latest/features/title.html

I did mention it above. The placeholder works by checking the children of the element provided as a container for the placeholder.

The idea is that toWidget() would set that custom property, meaning that you don't have to be aware of whether UIElement or RawElement works for you.

Can you elaborate on that? Where would that custom property be set? On each element? (e.g. by setting it to true by default on base view/Element class?).

niegowski added a commit that referenced this issue Feb 2, 2021
Fix (engine): The editor placeholder should not disappear until started typing. Closes #8689.
Fix (image): Image caption placeholder is now hidden when focused. See #8689.
Fix (heading): In the title plugin, the body placeholder is visible even when the body section is focused. See #8689.
Fix: Editor will show the placeholder even when focused. See #8689.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:engine package:heading package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants