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

BUGFIX: Restore old Placeholder behavior to keep shown when focused #3864

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Oct 9, 2024

In Neos 7.3.17 and before the placeholder in ckeditor would not disappear when focused. And only when typing. By switching to the native placeholder feature of ckeditor in #3558 we introduced a regression as that implementation would hide the placeholder while just clicking into. This is especially noticeable for centered texts as the cursor needs one millisecond to adjust and its glitchy.

This will eventually be fixed in Ckeditor 24 something. As a hotfix for our version 16 i introduce the patch of ckeditor/ckeditor5#8474 (without the css absolute change as that would make centered text uneditable). The mentioned fix was not merged directly into ckeditor but made way more complicated: ckeditor/ckeditor5#8867
We dont need the more complicated fix as we dont use the mentioned features of the placeholder in a image description and such.

before (placeholder might disappear fully and text is not editable unless clicking on another element)

Bildschirmaufnahme.2024-10-09.um.09.41.53.mov

after

Bildschirmaufnahme.2024-10-09.um.09.38.53.mov

What I did

How I did it

How to verify it

In Neos 7.3.17 and before the placeholder in ckeditor would not disappear when focused. And only when typing. By switching to the native placeholder feature of ckeditor in #3558 we introduced a regression as that implementation would hide the placeholder while just clicking into.
This is especially noticeable for centered texts as the cursor needs one millisecond to adjust and its glitchy.

 This will eventually be fixed in Ckeditor 24 something. As a hotfix for our version 16 i introduce the patch of ckeditor/ckeditor5#8474 (without the css absolute change as that would make centered text uneditable). The mentioned fix was not merged directly into ckeditor but made way more complicated: ckeditor/ckeditor5#8867
 We dont need the more complicated fix as we dont use the mentioned features of the placeholder in a image description and such.
@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.3 labels Oct 9, 2024
Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by reading. Will test later!

@Sebobo
Copy link
Member

Sebobo commented Oct 9, 2024

Works as expected, though my cursor (In Vivaldi / Chrome) is at the end of the placeholder which is confusing:

CleanShot 2024-10-09 at 11 15 17@2x

In Safari it works as in your video.

@markusguenther
Copy link
Member

I looked bit into the latest CSS changes of the Placeholder view, but there was nothing special. So I adjusted the position. Now we have the cursor at the first position.

Safari:
Screenshot 2024-10-11 at 14 28 40

Chrome:
Screenshot 2024-10-11 at 14 28 47

Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, it works like I would expect it.
Thanks for taking care @mhsdesign 💙

@mhsdesign mhsdesign marked this pull request as draft October 13, 2024 12:48
@mhsdesign
Copy link
Member Author

Thanks for the adjustments ... i dared to try in firefox but well:

image

The placeholder dropped out.

Also on safari it doesnt blink anymore but the styling looks good at least.

This obviously needs a bit more of love. Ill see how it behaves in CK-Editor 40 or something here #3863 just for testing ;)

@markusguenther
Copy link
Member

Hmm, ok the Firefox renders the br a bit different, and it seems the after position overlays the span, and so we don't see the cursor blinking. Hope to find some time tomorrow or Tuesday 🙈

To move the cursor to the first position, the placeholder text is moved backwards. Therefore, a change with a break is disadvantageous.
@markusguenther
Copy link
Member

markusguenther commented Oct 14, 2024

Tested the behavior in Firefox (132), Chrome (129) and Safari (18.0.1) and it behaves the same.

chrome-ff.mp4

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For fine for me now in latest Firefox (after updating), Vivaldi (Chrome) and Safari.

@markusguenther markusguenther marked this pull request as ready for review October 14, 2024 15:59
@markusguenther markusguenther merged commit 288e29a into 8.3 Oct 14, 2024
9 checks passed
@markusguenther markusguenther deleted the bugfix/fix-placeholder-disappearing-on-focus branch October 14, 2024 15:59
@mhsdesign
Copy link
Member Author

Thank you so much for carrying this one @markusguenther <3

laurahaenel pushed a commit to laurahaenel/neos-ui that referenced this pull request Oct 21, 2024
…eos#3864)

* BUGFIX: Restore old Placeholder behavior to keep shown when focused

In Neos 7.3.17 and before the placeholder in ckeditor would not disappear when focused. And only when typing. By switching to the native placeholder feature of ckeditor in neos#3558 we introduced a regression as that implementation would hide the placeholder while just clicking into.
This is especially noticeable for centered texts as the cursor needs one millisecond to adjust and its glitchy.

 This will eventually be fixed in Ckeditor 24 something. As a hotfix for our version 16 i introduce the patch of ckeditor/ckeditor5#8474 (without the css absolute change as that would make centered text uneditable). The mentioned fix was not merged directly into ckeditor but made way more complicated: ckeditor/ckeditor5#8867
 We dont need the more complicated fix as we dont use the mentioned features of the placeholder in a image description and such.

* TASK: Move cursor before the placeholer

* TASK: Hide break in placeholder context

To move the cursor to the first position, the placeholder text is moved backwards. Therefore, a change with a break is disadvantageous.

---------

Co-authored-by: Markus Günther <[email protected]>
@mhsdesign
Copy link
Member Author

Btw for headlines, something is still off but that was before as well i think:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants