Syncing the editor viewport scroll

markdown-it-review

(Robin Ward) #22

I agree that adding a cursor element to the rendered HTML is the best approach for syncing the viewport, but it will be challenging as we don’t pre-parse HTML so it’s not simple to know if we’re within an HTML tag or not.

You would have to check the previous text element in the jsonML and count the opening < versus closing > to ensure you didn’t break the HTML.


(Wojciech Zawistowski) #23

I’ve published a proof of concept of the possible solution to synchronised scrolling:

https://github.com/discourse/discourse/pull/2100

Probably not fully ready for merging into production yet (performance and some edge cases are not fully polished yet), but please check it out if an algorithm I implemented behaves in an acceptable way.

In short how it works:

  • invisible markers are inserted between all the primary “blocks” (paragraphs of text, images, oneboxes, citations etc.) of source content in such a way that they are compiled into matching markers in the preview HTML
  • when scrolling source pane, it is determined (via hidden DIV) which of these markers are currently “visible” in the pane
  • preview pane is scrolled accordingly (to show the same hidden markers) plus also relative proportions between two source and preview markers are taken into consideration

Currently I’ve taken such approach that when scrolling down, the bottom of the preview pane is synced with what is visible at the bottom of the source pane and when scrolling up, the top of the preview pane is synced with what is visible at the top of the source pane - but the markers based algorithm allows also different approaches, like always syncing the top, no matter of scrolling direction or e.g. always syncing the content in the middle of the pane.

Please take a look and give some feedback. If you think it looks ok, I’ll polish the code a bit more.


(Jeff Atwood) #24

I deeply regret that @sam insisted people work on this very hard and complex problem without considering the simple solution first. But thank you for taking it on, this looks like a lot of work and code.

I hope editor performance does not get affected too much, particularly for the common short posts where viewport sync is 100% irrelevant like this one. We already have several front page complaints about Discourse slowness, perhaps HTMLbars will help in a month or so.


(Sam Saffron) #25

This is completely unrelated to HTMLBars.

@velesin can you measure the perf implications of the change for both small and long posts?

Thanks for the help, will check this out today.


(Jeff Atwood) #26

I meant I am tired of people complaining about our front page / topic performance being terrible – which HTMLBars will help with.

I would hate to see editor performance degrade so much due to this advanced scroll detection and sync code that they have to now complain that our editor is slow as well.


(Sam Saffron) #27

I just tried this out @velesin and I am quite impressed, the scroll sync is very cool! @JohnONolan should definitely check this out as well.

I found it worked well even when you have images and oneboxes with one big caveat that needs addressing:

Take this content for example: https://meta.discourse.org/raw/13045

If you start typing after the image or onebox, it does not catch up properly.

The scroll sync is pretty flawless though (and not much of a perf issue @codinghorror cause the code only kicks in when you are scrolling a long post. )

Ping me when you get this addressed. Overall I am really happy with this.

I just had a look through Discourse General Polish prior to V1 and we have knocked quite a few items off :slight_smile: while in the composer can you have a look at the rendering bug after reloading a large draft (paste in the “raw” from above wait till post saves as draft, reload page)


(Wojciech Zawistowski) #28

There are two completely different parts related to performance:

  1. the performance of the markdown parser (it will be a little slower, because there are a few string replacements added on top of the current parser in order to insert markers for scrolling). There is not much room for optimization here (only thing that comes to my mind at the moment is checking if the source text is long enough to be scrolled, and if it is too short skip the marker insertion at all). However, I think the simple text replacement may be much faster than the full markdown parsing and so it may not add that much to the overall time to be noticeable (especially as this happens only on text update when typing, not on scroll) - I may try to measure these times to be sure
  2. the performance of scrolling. This is not optimized at all right now (it fires on every onscroll event and it recalculates everything from scratch each time), so there is a room for improvement here. First two ideas that come to mind are using throttling (so scroll syncing doesn’t happen on each onscroll but e.g. every 15-30 msec) and caching the top positions of markers, so there is no need to traverse the DOM on each update. I didn’t want to spend time on these optimizations before knowing that the solution goes in the right direction, but as you are happy with it, I’ll apply them.

Right now sync happens only on scroll - I’ll fix it to fire also on edit / paste. Although syncing currently can be done to some arbitrary position inside the pane (e.g. source top to preview top, bottom to bottom or center to center) - so I don’t know if simply firing the sync on edit will help in all the cases. To make sync perfect in such case it’d require detecting the caret position as @eviltrout proposed, but it may be quite tricky - I’ll play with it a bit and see whan can be done here.

OK


(Jeff Atwood) #29

I think these two in combination would meet my needs just fine; then I don’t have to worry about perf issues.


(Wojciech Zawistowski) #30

One more thing: right now I’ve chosen the simplest route and just hacked the Editor code (doing this outside of the Editor may be quite hard as I needed to plug into the markdown parsing pipeline). However, Editor is an external library, placed in vendors dir so it probably shouldn’t be modified (or at least it should be modified in such a general way that the changes can be pushed upstream to the original library).

On the other hand, I see that you already hack Editor’s code with a Discourse specific stuff (not to the extent I’m doing it, but there are already some parts of the code commented out etc.) - so maybe it is not a problem?

Also, right now I’m using jQuery for detecting elements positions while original Editor code uses only plain JavaScript.

So the question is, what is your policy regarding this:

  1. can the Editor’s code be modified at all (esp. with Discourse specific code)
  2. if it can, can it also use other libs (e.g. jQuery for positioning/events an lodash/Ember for throttling) or should it use only plain JS?

(Sam Saffron) #31

Yes, completely, and if we need to upstream any hooks then fine.

Either / or is fine. But perf is a major concern.


(Wojciech Zawistowski) #32

I’ve updated the PR

https://github.com/discourse/discourse/pull/2100

Please check it out. (More detailed info in the PR’s comment).


(Sam Saffron) #33

I just want to say I am super impressed with the implementation @velesin has come up with, definitely something others should emulate.

Better than stackedit which was touted as best in class till this popped up.

cc @JohnONolan


(JohnONolan) #34

Wow - that looks great!


(Sam Saffron) #35

I fixed this issue we had:

https://github.com/discourse/discourse/commit/f7644e2cdfd54155b3f02ba63cc21c4316d55ea9

In particular, loose lists … like this were bust in preview and code blocks were bust:

1. hello 

2. goodbye

3. boom
  1. hello

  2. goodbye

  3. boom

But … this regresses onebox support so the fix is no good.

In the interim I have disabled preview sync until we can clean up the pipeline.

In particular, layering on stuff to baked html is done in the wrong spot. The converter should be the one adding the markers and data returned from the converter should be untouched. This is both more efficient and less fragile. This means you need to work through markdown js and ensure it is extensible in this way. Intermediate presentation should have line# metadata, final render should add the markers.

Additionally there needs to be some logic to deal with dropped markers cause that will happen. eg multiline <!-- -->

No rush on this feature, we can not afford to regress any functionality for it.

Markdown js extensiblity is a priority for us anyway so improving it is something that can help us in other cases anyway.


(Jeff Atwood) #36

I believe @sam has a different solution in place now.

Test it out yourself: paste in a lot of text then try scrolling up and down in the preview.

I also noted it doesn’t seem to work reliably with keyboard in Chrome due to that (still!) bug with the editor toolbar buttons at the top of the editor. But it works fine in Firefox.


(Sam Saffron) #37

Markdown it does a pretty good job here:

https://markdown-it.github.io/

Another one to review when we eventually move to it.


(Jeff Atwood) #38

(Sam Saffron) #39

(Sam Saffron) #40

Discussed this with @tgxworld we probably want to borrow the implementation in the markdown it demo, scroll sync is still way off when images get involved.


(Alan Tan) #42

I merged the following commits in yesterday


The design itself was adapted from the the markdown-it demo with some tweaks. I’ve tested with a few sample posts locally which includes images/onebox and it syncs pretty well. If you run into any weird syncing problems with the input and preview, let me know with the content of the post that you’re syncing and I’ll have a look.