Add placeholder functionality to Editable component #475

ellatrix · 2017-04-20T20:51:09Z

To test, write in an image caption and make sure the placeholder appears when the field is empty, disappears when the field has text.

aduth

I wonder what's the accessibility impact on fake placeholders 🤔

aduth · 2017-04-20T21:59:20Z

blocks/components/editable/index.js

 		editor.on( 'NewBlock', this.onNewBlock );
 		editor.on( 'focusin', this.onFocus );
+		editor.on( 'input', this.onChange );


I'm concerned about the performance impact of binding to input in combination with getContent, which will become even worse with #466.

Without any content processing, and with the content size being so small, I think this is fine if we keep it in mind. For blocks like freeform, less so.

My concern here is that this fires too often which will cause the updateContent to trigger often which can create focus jumps while typing.

Tested this branch and I'm already having focus jumps while typing. These are really hard to solve in my exp with the per-block prototype.

I'll have another look at this in a bit.

aduth · 2017-04-20T22:00:57Z

blocks/components/editable/index.js


 		return (
 			<Tag
 				ref={ this.bindNode }
 				style={ style }
-				className={ classes } />
+				className={ classes }
+				placeholder={ placeholder } />


Wouldn't placeholder be an invalid attribute for non-input/textarea elements? Maybe data-placeholder instead?

I did a quick check and was unable to find an answers so far for contenteditable. data-* is probably safer.

aduth · 2017-04-20T22:01:33Z

blocks/components/editable/index.js

-		const { tagName: Tag = 'div', style, className } = this.props;
-		const classes = classnames( 'blocks-editable', className );
+		const { tagName: Tag = 'div', style, className, value, placeholder } = this.props;
+		const classes = classnames( 'blocks-editable', className, { 'is-empty': ! value } );


This and the above onChange logic on value will need some reconsideration if #466 is to come to be.

BE-Webdesign · 2017-04-24T16:20:15Z

blocks/components/editable/style.scss

+		position: relative;
+	}
+
+	&.is-empty:before {


Should the ::placeholder pseudo element be used here? The browser support is not good, so ::before is probably best?

@BE-Webdesign Huh, I wasn't aware of that pseudo-element. You're probably right on the point about browser support. Out of curiosity, for those browsers that do support it, would it work on non-input types like how we're using it here?

After looking it over, I believe it only exists on <input>, and <textarea>. I forgot that we are using content editable lol. ::placeholder and various vendor prefixes are often used to create consistency across the various browsers for the way they display the placeholder text, so I think ::before definitely makes the most sense now, disregard my comments :). I don't think you can set the content as a property for ::placeholder, so even if using it made sense it won't help us at all.

youknowriad · 2017-04-26T17:05:08Z

blocks/components/editable/index.js

 		}
-		const value = this.editor.getContent();
-		this.editor.save();
-		this.props.onChange( value );


I think relying on the dirty status is a good thing. More performant that value checks, especially after a rebase (which will update the shape of the value).

Are these changes mandatory for this PR?

If we sync changes on input, why would we save and dirty check?

ellatrix · 2017-04-27T15:12:34Z

#523 would be good to have for this, so we might be able to sync changes as the DOM changes, instead of just on blur.

ellatrix · 2017-05-02T15:50:19Z

Since #523 is merged, I had another look at this, but it turns out the captions are now an array of paragraphs. I'm not entirely sure if this is intentional, but it makes placeholders a bit more difficult to implement.

aduth · 2017-05-02T18:38:14Z

I'm not entirely sure if this is intentional

I might expect it's an array (based on behavior of children matcher), but of a mixture of text nodes and strong, a, and em tags, not paragraphs.

ellatrix · 2017-05-03T16:03:14Z

@aduth Yeah I'm not sure why it's paragraphs then. :)

ellatrix · 2017-05-04T10:14:17Z

The empty check feels like it should belong somewhere else... Like React vNode helpers or something. Or state selectors/helpers.

ellatrix · 2017-05-04T10:17:49Z

It is true that we are getting and setting bookmarks on updateContent... I feel like we should sync the selection here and reset it from state rather than injecting a DOM node. Looking.

ellatrix · 2017-05-04T23:29:29Z

This last commit is very much a work in progress. Maybe it should be split off in another PR.

The idea is to continuously keep the state in sync. This means calling onChange on every change in the editor. Since the content is the same when the new props come in, there is no need to set the content. It is only set if it's different.

Having the content in sync is handy to show a placeholder because we can directly check the props.

Keeping a simple selection state has several benefits. We don't need to set a bookmark as we can derive a new range from this state. If new content comes in, and the selection doesn't match what was selected before, it will be reset.

You can try this by e.g. changing the heading levels. New content comes in, old selection is applied.

Another cool side effect is that some checks become quite simple. Checking if the caret is at the start of the editor: this.selection.isCollapsed && ! compact( this.selection.start ).length. Similarly logic to split blocks.

Keeping a selection state will prove useful in the future as well when we start thinking about applying formatting.

ellatrix · 2017-05-05T12:09:33Z

There's an an error sometimes with the formatting, looking into that.

youknowriad

I like how this normalize how we store the selection. I think we could benefit from having this selection in the focus config in the state.

Some bugs I'm seeing:

After merging two text blocks, the selection should be collapsed
The link control is not working
Hovering a text block can select its content (not always)
Merging a text with a heading create 'br' tags

youknowriad · 2017-05-05T12:06:22Z

blocks/components/editable/index.js

+			return;
+		}
+
+		if ( ! isEqual( this.props.value, this.content ) ) {


I'm thinking the isEqual might not be super useful? Will this always be true?

It is not always equal, actually it will be more unequal as equal. No need to call onChange needlessly. Re selectionChange, I've found it be be one of the best events to pick up content changes. It always work regardless of the input method.

I think I'd raised it previously, but I'm concerned about the performance implication here. isEqual is a deep equality check which can be very slow, and here it's occurring on every keyboard event, selection interaction, focus, etc.

@aduth What would you suggest? Calling onChange all the time? We might want to do it anyway later if we set the selection too.

@aduth What would you suggest? Calling onChange all the time? We might want to do it anyway later if we set the selection too.

Personally I didn't see it being hugely problematic that we only triggered the change callback when removing focus from the block. Another issue we have with this is that UPDATE_BLOCK is repeatedly dispatched while typing. This might be ideal from the perspective of ensuring the Editable value and state are kept in sync, but is fairly wasteful to reconcile. For example, with these changes we're not only doing a deep equality here, but another needless one in componentDidUpdate.

I see what you're getting at, but I also don't think it's desirable to undo one character at a time. We'll probably want to consider some interval to update state (e.g. debounced change). Do you have any insight on how browsers or TinyMCE manage it?

Hm, I was hoping this is something we'd handle in the reducers, or dispatch ADD_UNDO_LEVEL.

Hm, I was hoping this is something we'd handle in the reducers, or dispatch ADD_UNDO_LEVEL.

I might not be communicating well, but I agree with you on this. I just don't think we need each keystroke to trigger an update to state; we can choose different intervals instead (focus out, after a 5s debounce delay, every new paragraph added, etc). Not something we necessarily need to flesh out here or now.

Or maybe we want to add some content transformations in the future, and things like checking empty content, which all need immediate feedback.

It could also be local state with local reducers? Would that be a strange thing? Not sure.

youknowriad · 2017-05-05T12:07:07Z

blocks/components/editable/index.js

-		this.editor.selection.moveToBookmark( bookmark );
-		// Saving the editor on updates avoid unecessary onChanges calls
-		// These calls can make the focus jump
+	getChildIndex( child ) {


Could this be changed to Array.from( parentNode.childNodes ).indexOf( child )?

youknowriad · 2017-05-05T12:14:55Z

blocks/components/editable/index.js

-		this.editor.save();
-		this.props.onChange( this.savedContent );
+		this.content = this.getContent();
+		this.selection = this.getSelection();


Maybe we should do something like this.props.onFocus( this.getSelection() ) instead of storing the selection locally. We'll get the selection back in the focus prop. And we'll be able to use it outside the Editable to perform the merge/split or any selection related action outside the editable itself.

Yep, I saw this as one of the next steps :)

…nentUpdate

ellatrix · 2017-05-10T13:57:13Z

I acknowledge that there are still a few range issues to be solved, and I have a good idea on how to solve these and keep on moving toward a more controlled component. It will require some extra things though, that may be better tried in a separate PR. I don't want to keep this issue open much longer. I'll adjust this so we only sync the state on blur and empty/fill, for now not when the formatting changes.

ellatrix · 2017-05-11T09:12:16Z

Closing, but please keep the branch for now.

aduth reviewed Apr 20, 2017

View reviewed changes

ellatrix added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 21, 2017

youknowriad mentioned this pull request Apr 24, 2017

Image Block: Fix adding an image caption to the image block #488

Merged

BE-Webdesign mentioned this pull request Apr 24, 2017

Support placeholder text for Editable component #378

Closed

BE-Webdesign reviewed Apr 24, 2017

View reviewed changes

youknowriad reviewed Apr 26, 2017

View reviewed changes

ellatrix added this to the Prototype Parity milestone Apr 26, 2017

mtias added the [Priority] High Used to indicate top priority items that need quick attention label May 1, 2017

mtias modified the milestones: May Week 1, Prototype Parity May 1, 2017

mtias added the [Feature] Blocks Overall functionality of blocks label May 1, 2017

mkaz mentioned this pull request May 2, 2017

Pullquote Block #607

Merged

ellatrix force-pushed the add/editable-placeholder branch from dbb6a10 to 69b128e Compare May 4, 2017 10:11

ellatrix force-pushed the add/editable-placeholder branch from 69b128e to 9569254 Compare May 4, 2017 10:16

ellatrix force-pushed the add/editable-placeholder branch from 9569254 to b5fdb00 Compare May 4, 2017 10:19

mtias added the [Feature] Block API API that allows to express the block paradigm. label May 4, 2017

ellatrix removed this from the May Week 1 milestone May 4, 2017

ellatrix force-pushed the add/editable-placeholder branch from b5fdb00 to 98be27b Compare May 4, 2017 23:13

ellatrix mentioned this pull request May 5, 2017

If possible, only input should fade out block boundaries #669

Closed

ellatrix force-pushed the add/editable-placeholder branch 2 times, most recently from 0046e02 to e24bdb7 Compare May 5, 2017 12:05

youknowriad reviewed May 5, 2017

View reviewed changes

ellatrix force-pushed the add/editable-placeholder branch from 92cd128 to a45f9d4 Compare May 9, 2017 22:10

ellatrix added 16 commits May 10, 2017 13:55

First try for placeholders

ae8ca4c

Sync content and selection to state

ac64bae

Use Array.from to get child node index

53e32e3

Make sure the editor is focussed before applying formatting

a94a380

Set local content state on setContent

1723d99

Simplify getSelection, fixes bug with <br>

00e812b

Invert comparison

1baeb37

Don't rely on globals to determine active state

7b59414

Only trigger change on TinyMCE change, and pass selection as well

930b462

Simplify isEmpty check and change state if it changes

1d2df5a

Fix focus when merging text blocks

8a5923b

Make isActive check actual focussed node

5679973

Content can be null

e6e0266

Simplify selection setting

59ba2f3

Properly center caption placeholders

9470613

Move range logic to separate file

02b4fc2

ellatrix force-pushed the add/editable-placeholder branch from a45f9d4 to 02b4fc2 Compare May 10, 2017 11:55

Use componentWillReceiveProps for manipulation instead of shouldCompo…

d81a2b6

…nentUpdate

ellatrix force-pushed the add/editable-placeholder branch from 6ea0bef to d81a2b6 Compare May 10, 2017 12:01

Only sync changes on blur

ec72213

ellatrix mentioned this pull request May 10, 2017

Add Editable placeholders #756

Merged

aduth mentioned this pull request May 10, 2017

Apply placeholder on editable wrapper #758

Closed

ellatrix closed this May 11, 2017

ellatrix deleted the add/editable-placeholder branch May 19, 2017 11:20

yassine4999 mentioned this pull request Nov 15, 2023

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#22

Open

yassine4999 mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#35

Open

yassine4999 mentioned this pull request Jan 5, 2024

[Snyk] Security upgrade jest-dev-server from 5.0.3 to 7.0.0 yassine4999/gutenberg#36

Open

Add placeholder functionality to Editable component #475

Add placeholder functionality to Editable component #475

Conversation

ellatrix commented Apr 20, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 27, 2017

ellatrix commented May 2, 2017

aduth commented May 2, 2017

ellatrix commented May 3, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 4, 2017

ellatrix commented May 5, 2017

youknowriad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented May 10, 2017

ellatrix commented May 11, 2017

BE-Webdesign Apr 24, 2017 •

edited

Loading

youknowriad Apr 26, 2017 •

edited

Loading

youknowriad left a comment •

edited

Loading