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

fix wrong position at end of line #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Chrissi2812
Copy link

This fixes following bug with current cordsAtPos function
wrong-position

This should fix ueberdosis/tiptap#215

@Chrissi2812 Chrissi2812 marked this pull request as ready for review March 6, 2019 10:06
@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

This will break on IE and Edge (and possibly Safari) because they don't return meaningful coordinates for empty text ranges.

@Chrissi2812
Copy link
Author

Chrissi2812 commented Mar 6, 2019

Jeah I see now, the right and left values were the same and so the next if was always true.
I have removed the first block so always the negative lookup is used.

This are my results from manual testing it.

Browser master patched
Microsoft Edge 44.17763.1.0 ✔️
IE 11.316.17763.0
Firefox 64.0.2 ✔️
Safari 11.1.2 ✔️ ✔️
Chrome 73 ✔️

I'm using tiptap because I can't setup the dev environment of prosemirror (windows). Results should however be the same.

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

I have removed the first block so always the negative lookup is used.

Won't that cause a similar issue when measuring the position directly after a line break? The code that tried both sides was there to try and work around that problem.

@Chrissi2812
Copy link
Author

Jep that was broken.

I added a new argument that is set for the end of a selection.
Now both cases are handled correctly.

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

What is the idea behind passing zero there? It seems Range offsets move their end when it would be before their start, so this is equivalent to passing offset. So doesn't that get us back to the empty range browser support issue?

Also, how are you planning to pass that new argument, since this is an internal function?

@Chrissi2812
Copy link
Author

Next try 🙈
Now it works even in IE 11 🎉

There you would pass it:
https://github.com/ProseMirror/website/blob/master/example/tooltip/index.js#L35
Would become:

let start = view.coordsAtPos(from), end = view.coordsAtPos(to, true)

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

You didn't answer the question about how you're going to pass that parameter.

Passing Math.max(offset - 2, 0), offset - 1 seems like it'll not actually get the position that was given as argument, but the position of the character before that.

@Chrissi2812
Copy link
Author

Sorry for wasting time...

Also, how are you planning to pass that new argument, since this is an internal function?

It's exported...

You didn't answer the question about how you're going to pass that parameter.

... and in the example the parameter would get passed to the function like this:

let start = view.coordsAtPos(from), end = view.coordsAtPos(to, true)

Passing Math.max(offset - 2, 0), offset - 1 seems like it'll not actually get the position that was given as argument, but the position of the character before that.

Nevermind that was a stupid idea to fix the bug IE 11 had.
With 7905a32 it works in all browsers and should get the position of last character.

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

It's exported...

Not really. It's exported from that file, but not from the prosemirror-view package. The thing you're calling in your example is the EditorView method, not the function you added this parameter to.

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

In CodeMirror 6, I use a different branch for Chrome and Firefox to sidestep this issue in those browsers. That doesn't help much on other browsers, though.

@marijnh
Copy link
Member

marijnh commented Mar 6, 2019

Okay, now this is starting to make some sense, but requires a new parameter in the public API. I'm still interested in trying to fix this without doing that, or, if that fails, trying to define that parameter more cleanly (right now, it looks quite a lot like a one-off hack) — possibly by making it distinguish between finding the side of the character after or before the given position (when there are two such characters). That'd also require making sure we get the DOM node at the right side from domFromPos).

I'll put some more thought into this tomorrow.

@Chrissi2812
Copy link
Author

if you find a cleaner way, or even a solution witch doesn't need an argument that would be even better.

I'm not so used to this js text selection range things, but trying my best to help 😅
Big thank you for your help with this 👍🏽

@marijnh
Copy link
Member

marijnh commented Mar 7, 2019

Could you see how things work for you with patch e2cd163?

Browser testing that may at some point be useful to someone

A little more research that I might as well dump here in case someone else searches for it with exactly the right search terms: getClientRects and getBoundingClientRect behavior in text nodes around line wrap points in the various browsers. When asking for the rectangles (using a Range's getClientRect method) for the line-broken whitespace:

  • Chrome and Safari produce two zero-width rectangles before and after the break (the first at the end of the top line, the second at the start of the next line)

  • IE11 and Edge behave similar, but the first rectangle has a width of a few pixels.

  • On Firefox you only get one zero-width rectangle before the break

When querying the empty range after the break (which only Chrome and Firefox support at the moment), Firefox returns a zero-width rectangle before the break, and Chrome, more reasonably, returns a zero-width rectangle after the break.

When querying the rectangles for <br> nodes, all browsers produce only a single rectangle before the break, usually zero width except that on Firefox it has some minuscule, sub-pixel width.

@Chrissi2812
Copy link
Author

Chrissi2812 commented Mar 7, 2019

Now we have another problem 😖

Firefox / Safari / Edge

Selection Ends at linebreak: works
Selection Starts at linebreak: not working
wrong-position

Chrome

Selection Ends at linebreak: not working
Selection Start at Linebreak: working
wrong-position-2

The first selection ends at Offset 104
And the second selection starts at Offset 104

I dont't think that cordsAtPos can handle this correctly without knowledge of selection start.
Maybe a new method should be added like cordsAtSelection?


Can't get IE 11 however to run the examples page
image

@marijnh
Copy link
Member

marijnh commented Mar 7, 2019

Hm, right, the position directly at the wrap point reasonably corresponds to two screen positions—the end of the previous line and the start of the next line. Without an additional argument to coordsAtPos there's no real way to disambiguate between those two.

But even with such an argument (which would have to go through the RFC process), there is a complication related to bidirectional text—you can take the box around a nearby character (usually), but to figure out which side of that box you need to use you'd have to know the direction of the text at that point, which is very involved to compute.

I don't see myself having more time to spend on this in the near future, unfortunately. I've opened ProseMirror/prosemirror#899 and ProseMirror/prosemirror#900 to track these issues in the meantime.

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.

Wrong bubble position
2 participants