search

simplajs / simpla-richtext-behavior

Behavior to add richtext editing to an element
https://www.simpla.io
MIT License
6 stars 0 forks source link

Try keep cursor on left side of placeholder #16

Closed bedeoverend closed 7 years ago

bedeoverend commented 7 years ago

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):

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

👍 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 commented 7 years ago

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 commented 7 years ago

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

madeleineostoja commented 7 years ago

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 commented 7 years ago

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