#8689: The editor placeholder should not disappear until started typing

[oleq]

Suggested merge commit message (convention)

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 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.

---

• TODO: Check if The editor placeholder should disappear when typing, not when focusing the editor #8689 is also fixed in this

• TODO: Make sure this works with RTL.

---

Additional information

This is a draft as I pointed out in the original PR. It needs proper tests and documentation.

[maxbarnas]

@oleq, I had trouble understanding at first how the hideOnFocus works here https://github.com/ckeditor/ckeditor5/pull/8867/files#diff-4691af5a35179ee901044c054fabd91c8baffb422338eaaba2a9a57121869541R162 without checking a focus.

Tell me please, if I got this wrong, but we are assuming that the post-fixer was fired on change, therefore we can assume the focus is there, in the element, so the only thing left is to check the configuration option to decide whether we want to hide placeholder.

If that's correct, then maybe we could change the name of hideOnFocus in needsPlaceholder() to something less related to the config property, and more to needsPlaceholder() context? Maybe requireSelection or reverse it to keepPlaceholderWhenFocused? Or maybe I should just add a lengthy comment with an explanation? WDYT?

[maxbarnas]

Also, do you see a preferred way of testing this change? https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-image/src/imagecaption/utils.js#L30

[oleq]

@oleq, had trouble understanding at first how the hideOnFocus works here https://github.com/ckeditor/ckeditor5/pull/8867/files#diff-4691af5a35179ee901044c054fabd91c8baffb422338eaaba2a9a57121869541R162 without checking a focus.

1. First, we check if the element is attached. If not, well, it certainly does not need a placeholder.
2. Then (since attached), we check if it has some content. If so, again, there's no point in giving it a placeholder.
3. Then we deal with the document focus because we know the element is attached and without any content.
   1. If we don't care about the document focus (L162), the whole next point is bogus. We can add the placeholder to the element straight away (L170) since it is attached and empty.
   2. If we don't want a placeholder on document focus, we need to check if the document is focused (L169)
      1. And, if the document is focused, then we must make sure the selection is somewhere else (L177) before giving it element a placeholder (like in the Title plugin).
      2. And if the document is not focused, we don't care about the selection (because the user is not editing) so we add the placeholder to the element straight away (L170).

If that's correct, then maybe we could change the name of hideOnFocus in needsPlaceholder() to something less related to the config property, and more to needsPlaceholder() context?

I agree this is a bad name of the argument. I'd say considerDocumentFocus/notNeededWhenDocumentFocused + some lengthy explanation should do the trick.

[oleq]

Also, do you see a preferred way of testing this change? https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-image/src/imagecaption/utils.js#L30

Somewhere in ImageCaptionEditing plugins we'll need a test that focuses the image with an empty caption (placeholder shows up) then it moves the selection to the caption (the placeholder should disappear).

[niegowski]

I think that we also need some tests for the new option (inside the placeholder tests). Currently, I can see only new tests for needsPlaceholder.

Also, this PR is introducing a breaking change on enablePlaceholder and needsPlaceholder functions.

[maxbarnas]

Also, this PR is introducing a breaking change on enablePlaceholder and needsPlaceholder functions.

I wonder whether I should:

• Update the tests and note down that we are introducing a breaking change to the default behavior of enablePlaceholder()
• Reverse the default behavior back to a state where it hides placeholder on focus, but then we need to update the configuration of all the builds, to make them keep the placeholder on focus.

[oleq]

• Let's unify the config and avoid negations when passing config between helpers.
  • keepOnFocus.
• Let's update editors (classic, inline, etc.) with the new configuration.
• This way there will be no BCs in the API.