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.

Sign up for GitHub

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

Jump to bottom

Rich Text: Add missing keep placeholder on focus prop. #17439

Merged
merged 3 commits into from
Sep 15, 2019
Merged

Rich Text: Add missing keep placeholder on focus prop. #17439

merged 3 commits into from
Sep 15, 2019

Conversation

epiqueras
Copy link
Contributor

Closes #17405

Description

This PR fixes backwards compatibility issues that came with the unintentional removal of the keepPlaceholderOnFocus prop in RichText.

It does this by adding the prop back in.

How has this been tested?

It was verified that setting the keepPlaceholderOnFocus prop to true makes the placeholder still show when selected/focused without content.

Types of Changes

Bug Fix: Fix backwards compatibility issues arising from accidentally removing the keepPlaceholderOnFocus prop from RichText.

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.

@epiqueras epiqueras added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Rich text /packages/rich-text labels Sep 14, 2019
@epiqueras epiqueras added this to the Future milestone Sep 14, 2019
@epiqueras epiqueras self-assigned this Sep 14, 2019
@gziolo gziolo added the [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone label Sep 14, 2019
@gziolo gziolo modified the milestones: Future, Gutenberg 6.5 Sep 14, 2019
gziolo
gziolo reviewed Sep 14, 2019
View reviewed changes
packages/block-editor/src/components/rich-text/index.js
@@ -336,6 +336,7 @@ class RichTextWrapper extends Component {
didAutomaticChange,
undo,
placeholder,
keepPlaceholderOnFocus,
Copy link
Member

Choose a reason for hiding this comment

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

Should we include the deprecation message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not actually deprecating this one.

Copy link
Member

Choose a reason for hiding this comment

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

We need to restore documentation then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#17445

gziolo
gziolo reviewed Sep 14, 2019
View reviewed changes
packages/block-editor/src/components/rich-text/style.scss Outdated
display: none;
&.is-selected {
&.keep-placeholder-on-focus [data-rich-text-placeholder] {
pointer-events: none;
Copy link
Member

Choose a reason for hiding this comment

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

Will it display a different cursor icon? I guess, it still needs to be overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shows the same one for me.

Copy link
Member

Choose a reason for hiding this comment

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

Why does this rule need to be added? Is the one above not enough on :after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed so that you can select/see the caret through the placeholder.

@ellatrix
Copy link
Member

ellatrix commented Sep 15, 2019 •
edited
Loading

Fixed #16733 (comment) in 37e6fab.

@ellatrix
Turns out the cursor prop is not needed if the pointer-events prop is…
46b2501 
… on the span instead of on the pseudo element
@ellatrix
Copy link
Member

I see one small issue: when the placeholder is kept on focus and you click on it, there is no caret visible. You can still type though. I see that the browser selection is somewhere after the placeholder, so we could enforce selection to be in the placeholder text element. Let's look at this separately.

@ellatrix ellatrix mentioned this pull request Sep 15, 2019
Closed
@epiqueras
Copy link
Contributor Author

Sounds good, thanks for taking a look at this!

@epiqueras epiqueras merged commit 4f115c7 into master Sep 15, 2019
@epiqueras epiqueras deleted the fix/rich-text-missing-keep-placeholder-on-focus-prop branch September 15, 2019 11:15
@epiqueras epiqueras mentioned this pull request Sep 15, 2019
Merged
5 tasks
@afercia
Copy link
Contributor

afercia commented Sep 16, 2019

I see one small issue: when the placeholder is kept on focus and you click on it, there is no caret visible. You can still type though. I see that the browser selection is somewhere after the placeholder, so we could enforce selection to be in the placeholder text element. Let's look at this separately.

This was also noted on #16733 (comment)

When keepPlaceholderOnFocus is true, the editable is partially non-clickable, because the click actually happens on the new <span> .

@ellatrix
Copy link
Member

When keepPlaceholderOnFocus is true, the editable is partially non-clickable, because the click actually happens on the new <span> .

I'm a bit confused. If the placeholder (the span element) is not clickable, how can the click happen on the span element?

I think the placeholder should be ignored/"invisible" for clicks, and instead fall in the parent element. I'll have a look to fix this. In the meantime it might be worth creating a separate tracking issue for it.

@afercia
Copy link
Contributor

afercia commented Sep 16, 2019

I'm a bit confused. If the placeholder (the span element) is not clickable, how can the click happen on the span element?

Language barriers 🙂 I mean it's not possible to click the field because the <span> sit on top of the field. I think this was fixed in #17439 by using pointer-events: none ?

@ellatrix
Copy link
Member

Yes, it should be fixed by this PR right?

@ellatrix
Copy link
Member

The only remaining issue I see is that the caret is hidden after the placeholder is clicked. It seems to be positioned after the placeholder instead of the text node before.

@ellatrix
Copy link
Member

Btw, this seems to work fine in Safari. In Chrome the caret becomes invisible.

@ellatrix ellatrix mentioned this pull request Sep 16, 2019
Merged
5 tasks
@johnstonphilip johnstonphilip mentioned this pull request Jun 30, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

@ellatrix ellatrix ellatrix approved these changes

@daniloercoli daniloercoli Awaiting requested review from daniloercoli

@etoledom etoledom Awaiting requested review from etoledom

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

Assignees

@epiqueras epiqueras

Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text [Release] Do Not Punt Used to indicate the issue or pull request must not be moved from the assigned milestone [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 6.5
Development

Successfully merging this pull request may close these issues.

RichText changes and custom blocks backwards compatibility for WP 5.3
4 participants
@epiqueras @ellatrix @afercia @gziolo