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

[Slate-React] Inline placeholder is treated as part of editor content #1344

Closed
blakeembrey opened this issue Oct 30, 2017 · 9 comments · Fixed by #2370
Closed

[Slate-React] Inline placeholder is treated as part of editor content #1344

blakeembrey opened this issue Oct 30, 2017 · 9 comments · Fixed by #2370

Comments

@blakeembrey
Copy link
Contributor

blakeembrey commented Oct 30, 2017

When rendering an inline placeholder, it's treated as part of the document and the cursor always appears after the placeholder (which doesn't feel natural). My thought was maybe this has something to do with rendering and Slate.js measurements including the placeholder for cursor positioning.

    function renderPlaceholder(props) {
      const { document } = props.editor.value

      if (document.text !== '' || document.nodes.size !== 1) return false

      return (
        <span
          contentEditable={false}
          styles={{ pointerEvents: 'none', userSelect: 'none' }}
        >
          {editor.props.placeholder}
        </span>
      )
    }
@ianstormtaylor
Copy link
Owner

@blakeembrey thanks for opening this.

The issue is we're kind of constrained by what CSS/HTML allow us to do here. We render the placeholder as a sibling of the node's inner content itself (before the content to be exact). By default, the placeholder has width: 0; white-space: nowrap; which ensures that it never messes up the selection, since it takes up no space.

An alternative would be to use position: absolute, but this then requires that the block has position: relative, which I didn't want to force users to add for the general case. And this also means that the placeholder is taken out of the layout.

I'm not sure there's a way to render contenteditable={false} inside the block and have it both take up space and not mess with selection events. I've tried a few different things, but I haven't done a super deep exploration, so I could be wrong. I'd be open to changes to how our default logic works if there's a way to handle the general case better.

@blakeembrey
Copy link
Contributor Author

Based on my limited knowledge of the project and some personal thoughts, here's an approach I would consider taking (not sure if it even matches up with the object model used by Slate.js - if not, I'm happy to create a separate project to demonstrate and flesh out the idea more thoroughly first).

First, make the placeholder part of the model itself. This could have interesting side-effects later down the line for different use-cases, but if each object in the model manages selection events for itself the "placeholder" object could say "on selection move the selection to the beginning of the element". This would avoid the need entirely needing to have zero width and it'd just be regularly selectable - just the selection event would immediately change (like it is today anyway) to where it says it should go based on selection type.

Once you have the "placeholder" node available, if there's a "render" change (e.g. normalize step that occurs after input) and it's part of the editor stack, the final listener can hook into it and automatically append the placeholder when no other nodes exist. On normalization after input, it can strip itself also from the total value.

Honestly, not sure if anything I'm saying is making any sense and I haven't played enough with building something myself, so maybe I'll need to make that demo anyway 😄

@ianstormtaylor
Copy link
Owner

@blakeembrey I'm not sure I want to make a placeholder node part of the model itself though. It seems like that means extra normalization logic, etc. for something that isn't really part of the "document" data at all (at least in the general case). For most people the placeholder is simply a styling concern that should be handled in the rendering layer, but not reflected in the data's "truth" itself (eg. it shouldn't be serialized ever, it shouldn't be "editable", it shouldn't affect selection behavior, etc.)

At least that's how I currently think about it.

That said... In this specific case, (ie. with this placeholder use case considered as an edge case that's not the general one), this is possible already by using an inline node with void: true. You could implement your own inline placeholder node that has these behaviors, and could even be made available as a plugin for others to use. If you wanted to flesh it out there that would be awesome.

But that sounds like it's solving different, grander goals than the original issue above. There's the pure rendering issue where the placeholder is currently offsetting the content. Then there's the "interesting side-effects" which can be separated from this. They could be combined and solved via "placeholder as data", but I think that would make the 90% case more confusing.

From what I can tell, adding the placeholder into the data model, and being forced to manage it on every change, seems like overkill for the 90% case. Instead, trying to find a pure HTML/CSS rendering solution to solve this behavior sounds better. This lines up with how the native DOM API's treat placeholders.

@blakeembrey
Copy link
Contributor Author

blakeembrey commented Oct 31, 2017

Ok, so did a bunch of digging. The cursor jump is caused by https://stackoverflow.com/questions/27786048/why-is-my-contenteditable-cursor-jumping-to-the-end-in-chrome - when placed next to the contentEditable=false and having no space, it'll continue encountering the jump.

One workaround would be to move the placeholder out of the Content - is there a reason it's inside a Node today?

Edit: I see the jumping is probably why you put the placeholder before the node children instead of after too.

@blakeembrey
Copy link
Contributor Author

FWIW, if isComposing is exposed as part of the value I can fall back to my old approach. The way I did this before renderPlaceholder was just my suggestion above. I had a custom wrapper div and would render my own placeholder that focused on click. E.g.

      <div className={classnames(styles.container, className)}>
        <Editor
          state={this.state.state}
          plugins={this.props.plugins}
          ref={editor => (this.editor = editor)}
          className={classnames(styles.editor, editorClassName)}
          onBlur={onBlur}
          onFocus={onFocus}
          onChange={change => this.onChange(change)}
        />
        {!value ? (
          <div
            className={classnames(styles.placeholder, placeholderClassName)}
            onClick={() => this.editor.focus()}
          >
            {placeholder}
          </div>
        ) : (
          undefined
        )}
      </div>

@zhujinxuan
Copy link
Contributor

zhujinxuan commented Apr 30, 2018

I think perhaps we can do tricks in onSelect to move cursor back if there is placeholder but no content exists?

I will have a look at draft-js to see how they work:

Update: draft-js let the editor as "position:relative", then placeholder as "position:absolute"

But I think, if value is empty, then we can render an empty div with "position:relative", then wrap the placeholder in that div

@zhujinxuan
Copy link
Contributor

Update: I cannot reproduce it. It seems as long as you have a block in the beginning (even it is empty), the cursor will move to the right place...

@bryanph
Copy link
Contributor

bryanph commented Jul 26, 2018

@zhujinxuan draft.js renders it outside of the actual content though. The problem here is that it is mixed with the contenteditable content, causing problems.

Any updates on this problem?

@bryanph
Copy link
Contributor

bryanph commented Jul 26, 2018

Oh actually, when adding pointer-events: none; the problem goes away now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants