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

Page down does not work on consecutive very long lines #4562

Open
webzwo0i opened this issue Dec 12, 2020 · 9 comments
Open

Page down does not work on consecutive very long lines #4562

webzwo0i opened this issue Dec 12, 2020 · 9 comments
Assignees
Milestone

Comments

@webzwo0i
Copy link
Member

Create a very long line (it does not need to contain 2000 characters, I used 1854)
Copy that line into a second line and a third line
Go to the beginning of the pad (ctrl+home)

Pressing PageDown does not have any effect.

A pad that has the problem is at https://video.etherpad.com/p/t-E5ftI15ZD7FOc35rGR

The "long" lines are wrapped and it seems that the bug is only present, when the line that follows the line that contains the caret is also very long. https://video.etherpad.com/p/AFNCmX3DxH1SAe-G6dSq shows an example of two long wrapped lines. On my screen the last line I can see is the one that contains a lot of "I see I see I see". I cannot see the line that contains "invisible invisible" etc. Pressing PageDown works. As soon as I add a letter at the end of the second line, it wraps again. Now, when I set the caret to the beginning, PageDown has no effect.

@JohnMcLear
Copy link
Member

JohnMcLear commented Dec 17, 2020

My query here then is if we still need our page up/down catch logic or if the bug has been caught upstream in contenteditable. Afaik it's a historic bug. Will take a look.

Pageup/down in Etherpad is not a nice experience due to latency.

@JohnMcLear
Copy link
Member

Oh boy, no. the pageup/pagedown experience in contentEditable is terrible in both Chrome and Firefox. Clearly something that needs addressing but probably has little drivers.

@JohnMcLear
Copy link
Member

JohnMcLear commented Dec 17, 2020

longPadIssue4562.zip

Attaching compressed .etherpad which has the issue, exported from video.etherpad.com just so we have in case..

Either way the bug is confirmed and something I can fix. I'm familiar w/ this code. I want to lint fix/refactor ace2_inner first tho so this wont be done in 2020

@JohnMcLear JohnMcLear self-assigned this Dec 17, 2020
@JohnMcLear
Copy link
Member

this is my next job btw

@JohnMcLear
Copy link
Member

Okay so I played around a bit and I have a new concept on expected User behavior.

  1. Disregard original rep.
  2. Get last line in viewport.
  3. Scroll view to one line below that line.

It's important we always disregard rep. This is the flaw Etherpad makes, we use the rep instead of the visible lines.

@webzwo0i
Copy link
Member Author

Nice! rep handling really deserves a "best practice" tutorial with tricks that can be learned by studying plugins. :-) (Or explained in a tutorial as in "Interesting concepts inside ACE")

@JohnMcLear
Copy link
Member

JohnMcLear commented Dec 26, 2020

The tricky thing is that if the rep of the line is longer than the visible viewport we can't go to that position..

Interestingly getVisibleCharRange never actually worked for anything where the line was longer than the viewport.....

Scroll.prototype.getVisibleCharRange = function (rep) {
  const lineRange = this.getVisibleLineRange(rep); // [0,0] which is correct
  return [rep.lines.offsetOfIndex(lineRange[0]), rep.lines.offsetOfIndex(lineRange[1])]; // is always [0,0] until you can see the last character then it's [0,line.length];
};

what should happen is that only the characters visible to the viewport (irrespective of rep) should be returned. rep in general was just a bad approach to solving the problem by the looks of things, I hope I didn't do it....

I seem to remember Google Docs does this with a shadow Dom the same way that I do ep_cursortrace but that is not the route I want to take here..

@JohnMcLear
Copy link
Member

@JohnMcLear
Copy link
Member

Just an update on this, @webzwo0i is doing the long line X offset logic and I did the rest of the page up/down stuff.

@JohnMcLear JohnMcLear assigned webzwo0i and unassigned JohnMcLear Mar 15, 2021
@JohnMcLear JohnMcLear added this to the 1.9 milestone Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants