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

Improved rich text placeholder #16733

Merged
merged 4 commits into from
Jul 26, 2019
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 24, 2019

Description

This small PR improves the currently clunky placeholder text for rich text instances.

The way placeholders are currently implemented is by "shadowing" the whole rich text element.

  • This may sometimes result in styles being duplicated (when using :before or :after). See the double line?

Screenshot 2019-07-24 at 16 15 43

  • Sometimes it needs style adjustments for multi-line instances (see removal of p handling).
  • It requires a wrapper tag with relative positioning.

When placeholders were first implemented, we used :before, but we cannot do that as themes may use pseudo elements for styling.

What I propose here is to insert a placeholder span element whenever then content is empty. This way the styling is always correct. It's as if the text is just there. This implementation requires less code, and it feel a lot "cleaner".

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

margin-top: 0;

& > p {
margin-top: 0;
Copy link
Member Author

@ellatrix ellatrix Jul 24, 2019

Choose a reason for hiding this comment

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

This is no longer necessary, since we're not duplicating the whole rich text element.

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Jul 25, 2019
@ellatrix ellatrix force-pushed the try/improved-rich-text-placeholder branch from c8334ee to 9254300 Compare July 26, 2019 10:59
@ellatrix ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jul 26, 2019
@ellatrix ellatrix force-pushed the try/improved-rich-text-placeholder branch from 4ae0da8 to 2410275 Compare July 26, 2019 12:09
@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended and removed [Status] In Progress Tracking issues with work in progress labels Jul 26, 2019
@ellatrix ellatrix added this to the Gutenberg 6.2 milestone Jul 26, 2019
@ellatrix
Copy link
Member Author

@jasmussen @kjellr Would love your review on this as you have worked a lot with themes before. This change should make placeholder more invisible when developing themes, so themes should never have to take them into account.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

From a theme perspective, this seems great. Definitely a positive move. ⭐️

I tested in a handful of existing themes, and saw no negative effects. The only placeholder that does still seem to perform oddly is the very first paragraph placeholder for an empty doc — this one is still rendered as a textarea, so it doesn't always inherit the correct text styles. This was existing behavior though, so it can be looked into separately.

@ellatrix ellatrix force-pushed the try/improved-rich-text-placeholder branch from 2410275 to 69cb29d Compare July 26, 2019 14:09
@ellatrix
Copy link
Member Author

The only placeholder that does still seem to perform oddly is the very first paragraph placeholder for an empty doc — this one is still rendered as a textarea, so it doesn't always inherit the correct text styles.

Sounds like we should use RichText there instead. :)

Let's iterate if any placeholders turn out to be a bit off, also for the core blocks. If we end up needing placeholders on focus again, we can use CSS for those specific blocks, or reintroduce the prop to enable this rule.

&.is-selected [data-rich-text-placeholder]::after {
	display: inline;
}

@ellatrix ellatrix merged commit b51e44f into master Jul 26, 2019
@ellatrix ellatrix deleted the try/improved-rich-text-placeholder branch July 26, 2019 17:02
@ktmn
Copy link

ktmn commented Aug 31, 2019

If we end up needing placeholders on focus again, we can use CSS for those specific blocks, or reintroduce the prop to enable this rule.

I definitely want placeholder on focus back. It's standard behavior as agreed here #806.

When I insert a block and it autofocuses the first Rich Text input then I don't see the placeholder at all. There's no blinking cursor either for some blocks, the .block-editor-rich-text has 0 width. I've no indication that I should type anything at all.

At least a prop for it would be welcome.

Edit: wait, there was a prop keepPlaceholderOnFocus, why was it removed?

@ellatrix
Copy link
Member Author

ellatrix commented Sep 2, 2019

@ktmn You might want to have a look at e.g. the button block, which is also collapsed if there is no content. I think the key is to set display: inline-block on a wrapper around the rich text element, and leave the rich text element display: block.

@ktmn
Copy link

ktmn commented Sep 2, 2019

That does add some width, but it's not necessary with:

.rich-text.is-selected [data-rich-text-placeholder]::after {
    display: inline-block;
}

But with that I cannot click on the [contenteditable] again after it's lost focus, unless:

  1. I click on the very left-most pixel of the placeholder (sometimes works)
  2. Remove pointer-events from [data-rich-text-placeholder] (then I can focus the rich text and type into it, but the blinking cursor won't show up)

So I feel like there needs to be some JS click delegation going on that I can't solve with CSS.

Can you solve it? And then bring back keepPlaceholderOnFocus prop and make it do that? Then this change would be backwards compatible.

@afercia
Copy link
Contributor

afercia commented Sep 11, 2019

I'd like to ask the team where the removal of keepPlaceholderOnFocus is documented.

I can't find anything related in the changelogs or in the deprecations.

This prop removal is definitely breaking the interaction on some custom plugins blocks.

This kind of changes may appear safe in the context of Gutenberg but they're definitely breaking changes for plugins. They need to be documented and communicated well in advance of a WordPress release.

Not to mention the time necessary to debug issues produced by undocumented, breaking, changes is considerably high.

I'd also like to point out, as @ktmn mentioned, that the editable within custom blocks that use keepPlaceholderOnFocus is now partially non-clickable, because the click actually happens on the new <span> . This completely breaks the expected interaction.

See in the screenshots below: notice the mouse pointer indicates the "clickability" only on the right part of the editable, and see the non-clickable <span> highlighted in blue:

Screenshot 2019-09-11 at 12 27 34

Screenshot 2019-09-11 at 12 27 40

Screenshot 2019-09-11 at 12 29 11

@afercia
Copy link
Contributor

afercia commented Sep 11, 2019

The cursor type issue relates also to default blocks:

before:

Screenshot 2019-09-11 at 12 49 38

after:

Screenshot 2019-09-11 at 12 49 00

@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 13, 2019
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants