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

Try keep cursor on left side of placeholder #16

Merged
merged 2 commits into from
May 30, 2017

Conversation

bedeoverend
Copy link
Contributor

@bedeoverend bedeoverend commented May 29, 2017

This is an attempt to keep the cursor on the left side of placeholder.

Notable times when the cursor gets positioned on the incorrect side (checked means this PR addresses it):

  • When clicking on element, but not the placeholder widget (element not focused)
  • When clicking on element, but not the placeholder widget (element already focused)
  • When focused and using left / right keys to get to other side of widget
  • When focused and using CMD+Backspace to remove all content

The first two are different because the first can be checked as the selection on the state changes, the second however doesn't fire anything as according to ProseMirror's state (and everything else actually, including the browser) it considers the selection to be the same whether its on the left or right side of the widget.

They keyboard is fixed by just banning left / right keys when focused and has a placeholder.

Can't figure out how to stop the last instance from occurring. Plus because the left / right keys are banned, they can't move the cursor to the left of the placeholder. But that's minor as if they start typing the placeholder is removed anyway.

Also worth noting that because this has to dispatch new selections to keep the cursor to the left of the widget, there is a flash where the cursor jumps from right to left.

@bedeoverend
Copy link
Contributor Author

@seaneking could you pull this down and take a look at how it behaves? In some ways it looks more glitchy because of the jump.

@madeleineostoja
Copy link

Plus because the left / right keys are banned, they can't move the cursor to the left of the placeholder. But that's minor as if they start typing the placeholder is removed anyway.

I wouldn't call that minor, end result might be but it'd look pretty broken.

Also worth noting that because this has to dispatch new selections to keep the cursor to the left of the widget, there is a flash where the cursor jumps from right to left.

That's pretty shitty too.

What was wrong with just making placeholder widget 100% width?

@bedeoverend
Copy link
Contributor Author

Might be worth just closing this and start a fresh with a cleaner solution once we come up with one. Have explored quite a few different solutions and none work without making the cursor jump around the place.

Re: 100%, we discussed outside of GitHub, but just for posterity's sake - can't use 100% width for couple of reasons. It does reduce the occurrence as it's less likely you'll click outside of the placeholder, but in the other scenarios where this happens it's more pronounced because the cursor is all the way to the right of the element. Also because it's width 100% it means that typing on the left can push the placeholder to a new line before its removed, which looks even worse.

@madeleineostoja
Copy link

Okay, but this is still a breaking bug, IMO. It looks hella broken when using. I haven't actually tested this yet, so will pull it down quickly and see if it's at least better or worse than what's there now

@bedeoverend
Copy link
Contributor Author

At this point, breaking for me is: will users not use simpla-article or simpla-text because of this bug? Just because I've no strategy for resolving this at this point, so it would be a very long unknown rabbit hole of work.

Anyway, let me know what you think about the behaviour in this PR and we can go from there.

@madeleineostoja
Copy link

👍 Yep guess it's not technically breaking, just really shitty. But sure thang. If this was a regression on simpla-text (need to confirm that it's not, actually) then it'd be breaking in the sense of big enough not to release with new behavior, but since article is a new thing anyway yeah we could probably release with it. Just a pretty ugly wart.

@bedeoverend
Copy link
Contributor Author

If it is a regression on simpla-text, then that means there'll be a solution which is nice. The underlying logic of placeholder hasn't changed between this and the old simpla-text - it was just copied over to the new behavior.

@bedeoverend
Copy link
Contributor Author

I haven't checked either actually, should have done that, I just assumed it wasn't - never a good idea.

@madeleineostoja
Copy link

Okay tested this and I'd say it's an improvement and worth merging in for now, but yeah definitely not a long-term solution, for a start removing content from the first line is a pretty big edge case, and that leaves the cursor stuck at the end of the placeholder (no matter how you remove content, FWIW, not just cmd+backspace).

So let's merge and leave that issue open as a major bug, which needs a solution pretty soon.

@madeleineostoja
Copy link

madeleineostoja commented May 30, 2017

And yes, this is a regression on simpla-text, so I'll make a new issue and mark it as major. It'd be breaking if this wasn't merged, but with this merged I'd say the placeholders are at least usable without seeming completely broken

@madeleineostoja madeleineostoja merged commit a34e435 into master May 30, 2017
@madeleineostoja madeleineostoja deleted the placeholder-selection-fix branch May 30, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants