Syncing the editor viewport scroll


(Jeff Atwood) #15

Wow, this feature is SO EASY! Just like @sam promised us! That’s why it got implemented so fast!

(Wojciech Zawistowski) #16

I’ve checked StackEdit - they indeed have a very accurate sync, but they do it in a relatively complex manner - both source and preview panes are advanced text editors that tokenize content in realtime - so then it can be accurately mapped which source tokens map to which result tokens, detect which source token is visible at the moment and adjust the preview pane to scroll to its counterpart.

In case of Discourse composer, source is just a plain text (and preview is also a plain HTML) so it’s not possible to apply similar approach on the top of existing composer - it would have to be completely redesigned.

But I wonder if some simplified version of such approach couldn’t be applied “behind the scenes” - e.g. copy source text to an invisible div, tokenize it somehow (maybe e.g. in a similar way as Ember does with metamorph script tags in templates) then render preview html based on tokenized instead of raw version and finally try to scroll the preview pane to a particular token/metamorph. It’s just a free thought right now, but I’ll take a deeper look into Discourse Markdown class to see what’s possible.

(Sam Saffron) #17

I am actually not offended at all by your hacky ways, I feel they are a totally appropriate quick solution when you have a huge viewport. however it falls really short in places where the viewport is very limited like Discourse. Love ghost, love what you are doing, you are kindred spirits to Discourse. Only thing I really wish is that you were using Discourse as your discussion/support platform :blush:

For the record, my hacky ways are often wayyyy hackier

(Sam Saffron) #18

My immediate thought here was to send the cursor position into markdown.js and have it add a special div into the preview where the cursor is.

Of course the initial concern with this approach is that you could be typing in the middle of a IMG SRC or some other non-displayable area. In which case since markdown js tokenises and parses I would just skip adding the magic div and not worry about sync there, it is an edge case.

Once you have a magic div in place positioning becomes trivial. All you need to do is ensure it is visible.

Second approach I considered was backwards matching the previous 2 words before where the cursor is, matching that in the preview html and inserting magic div there, it is a tiny bit more expensive and complicated.

I think one huge benefit here is utilising markdown js.

(JohnONolan) #19

We actually tried! That was Plan-A. The rest of is all Rails, too. I think we even have a repo somewhere with the initial implementation. Long story short: the SSO features weren’t there yet and we were having to hack out all the functionality that we didn’t want. We have our own users API with data, profiles, etc. It was a little too hard to get them to mesh together in a way that made sense. Might come back to it in the future though when we have more resources (and budget).

(Sam Saffron) #20

Sure, totally makes sense, our SSO story as of last week is pretty awesome and slowly approaching spectacular thanks to our awesome community battle testing it.

I would be more than happy to help you out, PM me if and/or when you need a hand.

(Wojciech Zawistowski) #21

Another concern is the situation when user is not typing but dragging the scrollbar - in such case cursor position stays unchanged (and goes outside the visible area) what will result in improper sync until user clicks inside the source textarea or types something on the keyboard, what IMO may look a bit glitchy. But in general I agree with your train of thoughts - I think about something similar to your idea with invisible marker, but I’m considering inserting such marker e.g. after each newline instead of only at the cursor position. I’ll research/prototype this more in-depth this week.

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

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:

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.


(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

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!