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

Pasting in content causes scroll to "jump" before going back to original position #1374

Closed
sferoze opened this issue Mar 22, 2017 · 81 comments
Closed
Labels
1.x only Issues that have been addressed in v2 bug:functionality

Comments

@sferoze
Copy link
Contributor

sferoze commented Mar 22, 2017

The Quill editor I am using auto grows as the user types. I also have multiple notes (Quill editors) on the screen at a time. I am using the scrollingContainer option correctly.

After pasting in content, Quill does resume the original scroll position but only after the screen flickers a bit. (This is because the scroll jumps to another location and then it set back right away, which causes the screen to "jump" or "flicker")

On mobile it is even worse, as the jump it very noticeable.

So the original scroll position is resumed but it looks very janky. Users will feel like the app is not solid with this side effect. Any advice on how to prevent this?

Steps for Reproduction

  1. Visit the quill demo page with auto grow and scrolling container
  2. Try pasting in content into the middle of the document. Try both on desktop and mobile
  3. You will notice the scroll area jump or flicker but then go back to it's original position.

Expected behavior:

When pasting in content, there should be no flicker, jumping, flashes or what not. It should be stable and just accept the content like pasting in Word, or any other text editing program.

Actual behavior:

The screen jumps or flashes when pasting in content, but after it resumes its original scroll position. The issue here is the flash or jump which makes the app feel janky.

Platforms:

Chrome and Safari

Version:

1.2.2

@sferoze
Copy link
Contributor Author

sferoze commented Mar 22, 2017

@jhchen I'm narrowing in on the culprit....

In the onPaste function below the line this.container.focus(); is what causes the scroll to jump.

If I comment out that line the scroll does not jump anymore, but also after pasting in content, the cursor is still at the beginning of the pasted content, and not at the end.

onPaste(e) {
    if (e.defaultPrevented || !this.quill.isEnabled()) return;
    let range = this.quill.getSelection();
    let delta = new Delta().retain(range.index);
    let scrollTop = this.quill.scrollingContainer.scrollTop;
    this.container.focus(); // THIS LINE CAUSES SCROLL JUMPING
    setTimeout(() => {
      this.quill.selection.update(Quill.sources.SILENT);
      delta = delta.concat(this.convert()).delete(range.length);
      this.quill.updateContents(delta, Quill.sources.USER);
      // range.length contributes to delta.length()
      this.quill.setSelection(delta.length() - range.length, Quill.sources.SILENT);
      this.quill.scrollingContainer.scrollTop = scrollTop;
      this.quill.selection.scrollIntoView();
    }, 1);
  }

@sferoze
Copy link
Contributor Author

sferoze commented Mar 24, 2017

@jhchen Good news, I fixed it! I think this fix should make it into the next update.

Here is the code I added. The code goes into the Clipboard Module.

I add code in 2 spots, in the constructor and in the onPaste function. Here is the constructor

class Clipboard extends Module {
  constructor(quill, options) {
    super(quill, options);
    this.quill.root.addEventListener('paste', this.onPaste.bind(this));
    this.container = this.quill.addContainer('ql-clipboard');
    // NEW CODE BELOW
    this.container.addEventListener('scroll', (e) => {
      e.preventDefault();
      e.stopPropagation();
      if (this.scrollTop) {
        this.quill.scrollingContainer.scrollTop = this.scrollTop
      }
      return false;
    });
    // END OF NEW CODE
    this.container.setAttribute('contenteditable', true);
    this.container.setAttribute('tabindex', -1);
    this.matchers = [];
    CLIPBOARD_CONFIG.concat(this.options.matchers).forEach((pair) => {
      this.addMatcher(...pair);
    });
  }

and the onPaste function

onPaste(e) {
    if (e.defaultPrevented || !this.quill.isEnabled()) return;
    let range = this.quill.getSelection();
    let delta = new Delta().retain(range.index);
    let scrollTop = this.quill.scrollingContainer.scrollTop;
    // NEW CODE BELOW... JUST THE ONE LINE
    this.scrollTop = scrollTop;
    // END OF NEW CODE
    this.container.focus();
    setTimeout(() => {
      this.quill.selection.update(Quill.sources.SILENT);
      delta = delta.concat(this.convert()).delete(range.length);
      this.quill.updateContents(delta, Quill.sources.USER);
      // range.length contributes to delta.length()
      this.quill.setSelection(delta.length() - range.length, Quill.sources.SILENT);
      this.quill.scrollingContainer.scrollTop = scrollTop;
      this.scrollTop = null;
      this.quill.selection.scrollIntoView();
    }, 1);
  }

As you can see I added a function to hook into the scroll event of the ql-clipboard element. When the clipboard element is focused, it calls the scroll event... and in the scroll event function I set the scrollingContainer to be the saved scroll position. This works to keep the scroll position stable while pasting.

Now, everytime I paste content, the scroll is completely stable. No jumping around or anything of the sort. Please let me know if you see any potential side effects, otherwise I recommend adding this change ASAP as the scroll jumping bug is pretty bad.

@deepakjtrigent
Copy link

deepakjtrigent commented Apr 14, 2017

@jhchen Found a solution which does not require scroll position manipulation.

The problem is focusing on clipboard which is located below the editor. Hence I absolute positioned the clipboard with height and width 100%, top and left 0 and made its z-index=-1.

This would place the clipboard behind the editor ensuring the focus on which does not scroll editor.

Apply the below CSS to ql-clipboard

.ql-clipboard {
    height: 100%;
    width: 100%;
    position: absolute;
    left: 0;
    top: 0;
    z-index: -1;
}

Also remove the scroll position manipulation on the onPaste event.

onPaste(e: any) {
    if (e.defaultPrevented || !this.quill.isEnabled()) return;
    let range = this.quill.getSelection();
    let delta = new Delta().retain(range.index);
    this.container.focus();
    setTimeout(() => {
      this.quill.selection.update(Quill.sources.SILENT);
      delta = delta.concat(this.convert()).delete(range.length);
      this.quill.updateContents(delta, Quill.sources.USER);
      this.quill.setSelection(delta.length() - range.length, Quill.sources.SILENT);
      this.quill.selection.scrollIntoView();
    }, 1);
  }

Please keep me in the loop if this is affecting anything else. Would be happy if the jumping issue is fixed which is persisting from a very long time.

@sferoze
Copy link
Contributor Author

sferoze commented Apr 14, 2017

@deepakjtrigent I just tried this out and it does not solve the issue on mobile devices...

And if you comment out the scroll position manipulation the scroll position jumps to a random spot on mobile.

If we keep the scroll position manipulation the scroll position jumps real quick and then return to the scroll position on mobile.

@deepakjtrigent
Copy link

@sferoze Ok. I had just tried it on desktop, not on mobile.

@jhchen
Copy link
Member

jhchen commented May 18, 2017

@sferoze thank you for digging into this and apologies for the delay. The volume of issues and notifications from random people poking on the internet unfortunately is such that that legitimate ones overlooked.

I tried out your suggested code and the jumping still persists on an iPad running Safari 10. Is it consistently working for you?

@sferoze
Copy link
Contributor Author

sferoze commented May 18, 2017

@jhchen no it is not working on mobile for me.

The code I added does fix for desktop but does not fully fix the issue on mobile. Thanks!

@jhchen
Copy link
Member

jhchen commented May 29, 2017

I don't have any new ideas on how to fix this on mobile. The two approaches are generally to avoid the flicker (which I don't think is possible at the moment) or to correct for it. The latter is the current approach but for whatever reason desktop browser it is not noticeable (perhaps performance).

@sferoze
Copy link
Contributor Author

sferoze commented Jun 8, 2017

@jhchen

There is a way to prevent the flicker on desktop. I still notice the flicker on desktop with the current version of Quill. I have to use my own custom edited version with this code added to the contructor of Clipboard module

    _this.container.addEventListener('focus', function (e) {
      e.preventDefault();
      e.stopPropagation();
      if (_this.scrollTop) {
        _this.quill.scrollingContainer.scrollTop = _this.scrollTop;
      }
      return false;
    });
    _this.container.addEventListener('scroll', function (e) {
      e.preventDefault();
      e.stopPropagation();
      if (_this.scrollTop) {
        _this.quill.scrollingContainer.scrollTop = _this.scrollTop;
      }
      return false;
    });

_this.scrollTop is the saved scrolltop position from the onpaste function.

If you add this code it will improve stability of quill on paste.

@guillaumepotier
Copy link

Hi there,

What is the status of this issue? Do you think a fix for desktop could reach next version even if no satisfying one for mobile found?

Thanks

@VioletPixel
Copy link
Contributor

In 1.3.0, I fixed this on both desktop (Safari 10, macOS 10.12) and mobile (Safari, iOS 10.3) by modifying the Clipboard module's onPaste() function. I added this:

this.container.style.position = 'fixed';
this.container.style.zIndex = '-1';

Immediately prior to this:

this.container.focus();

Which makes sure the paste destination is both in view (position: fixed) and not visible (z-index: -1), so no scrolling is required.

ximing pushed a commit to ximing/weditor that referenced this issue Aug 5, 2017
@jhchen
Copy link
Member

jhchen commented Aug 7, 2017

TLDR: No proposed solution seems to work better than the current one so this is still an open issue.

Quill's paste works by adding a paste listener and shifting focus to another contenteditable container before the paste happens, and shifting focus back to the real editor after paste. The pasted content is then analyzed and inserted into Quill through standard APIs. This has two benefits:

  1. The pasted content is isolated and cannot trash Quill's editor contents in unpredictable ways as the only way it gets there is through standard APIs.
  2. More importantly, the browser fully performs a paste, which contains extra content and context that is not available by just looking at the paste event object. We tried relying on just the event object in the past and reverted for this reason.

The focus back onto Quill causes it to scroll to the top. You can try in this demo: https://jsfiddle.net/p2pgrxdc/. Scroll the blue div down and paste some text into it. Observe it scrolls to the top. I am unaware of any way to prevent this. If there is a way that would be key to solving this issue.

So instead what Quill does is save the scroll position before the focus and restores it after. This causes a flicker on slower devices, like mobile. Personally on desktop I do not see any flicker but in theory this is perhaps I use a faster machine. Nevertheless for this reason it does for me make this issue harder to debug so I can only read the code and reason about it.

Unless I am misunderstanding the code, the suggestions by @sferoze still save and restore scroll positions and would still induce the flicker. Indeed this would explain why it still manifests on mobile. The preventDefaults do not appear to do anything but I may be missing something here. Otherwise, because the flicker is so fast on desktop to a point it is indiscernible on many environments, I would be reluctant in this situation to make core changes based off anecdotal suggestions alone.

@sferoze
Copy link
Contributor Author

sferoze commented Aug 7, 2017

@jhchen

The preventDefaults might not be doing anything but...

_this.container.addEventListener('focus', function (e) {

_this.container.addEventListener('scroll', function (e) {

aren't these even listeners earlier hooks to restore scroll? For me, the scroll jumps on desktop with original Quill, but when I added those lines there is no jumping on desktop

@jhchen
Copy link
Member

jhchen commented Aug 14, 2017

Hmm @sferoze can you try to place the line this.quill.scrollingContainer.scrollTop = scrollTop; first in the setTimeout block:

setTimeout(() => {
      this.quill.scrollingContainer.scrollTop = scrollTop;
      delta = delta.concat(this.convert()).delete(range.length);
      this.quill.updateContents(delta, Quill.sources.USER);
      // range.length contributes to delta.length()
      this.quill.setSelection(delta.length() - range.length, Quill.sources.SILENT);
      this.quill.focus();
    }, 1);

I believe this would make the restore run as soon as it possibly can.

@sferoze
Copy link
Contributor Author

sferoze commented Aug 16, 2017

@jhchen

I tried saving the scroll position in the setTimeout like you mentioned, instead of right before it.

No difference on desktop, but on mobile the jump is still visible. Saving the scroll position in the setTimeout actually makes the mobile jump a bit worse. So for now my custom fix remains the same.

I also tried @ValueBerry css fix and it does not work on mobile in my testing on iOS 10.3

still an open issue, and for now the code I posted in my testing does the best job of fixing destop, and making mobile jump as small as possible.

@sferoze
Copy link
Contributor Author

sferoze commented Sep 29, 2017

@jhchen why can we not set the scrolling container to window

$(window).scrollTop() always returns the proper scroll.

on chrome on mac $('body').scrollTop() has the scroll and on windows $('html').scrollTop() has the scroll.

different browsers put the scroll on body or html

but window has the scroll on all browsers.

@jhchen
Copy link
Member

jhchen commented Oct 5, 2017

why can we not set the scrolling container to window

Not sure what this means. Quill also does not use jQuery and cannot add it as a project dependency but now sure if the above misunderstanding would have clarified this example

@sferoze
Copy link
Contributor Author

sferoze commented Oct 5, 2017

Ah let me clarify

https://quilljs.com/docs/configuration/#scrollingcontainer

For the scrolling container option:

scrollingContainer

Default: null

DOM Element or a CSS selector for a DOM Element, specifying which container has the scrollbars

I was hoping to be able to pass window in as the scrolling container.

@jhchen
Copy link
Member

jhchen commented Oct 9, 2017

Not sure window is a DOM element but it is not the entity that has the scrollbars.

@magicdvd
Copy link

how about the status of this issue? It is a big defect.

@slab slab deleted a comment from wy930208 Oct 29, 2017
@gnemtsov
Copy link

gnemtsov commented Nov 4, 2017

For me all problems with scroll jumps were gone, after I had set
scrollingContainer: document.documentElement

@DmitrySkripkin
Copy link

DmitrySkripkin commented Jan 8, 2018

Another workaround. I added eventListener to scrollingContainer and move .ql-clipboard so it can be focused without scroll. This works for desktop and mobile. Maybe I didn't test it enough, but looks like 100% working solution.

@jhchen
Copy link
Member

jhchen commented Jan 18, 2018

@DmitrySkripkin Can you elaborate on your solution? Not sure I understand the adding eventListener part or the moving .ql-clipboard

@jrluiscarvalho
Copy link

Hello everyone!

I`ve had a "scroll jump" problem when i select a text with more of two paragraphs in mobile devices, android and IOS. I tried configuring "scrollingContainer" but not works. Any one have ideas for this solution?

@Chris-Kin
Copy link

Does the CSS in #1374 (comment) work for you two?

doesn't work for me.

@itsmhuang
Copy link

itsmhuang commented Oct 22, 2019

I set scrollingContainer to html and add this css:

.ql-clipboard {
  position: fixed;
}

it fixed the problem for me. try it out.

@huanlirui
Copy link

现在我解决了这个问题:
首先,设置 scrollingContainer="your scroll DIV DOM"
然后设置CSS
.ql-clipboard {
position: fixed !important;
left: 50% !important;
top: 50% !important;
}

Now I've solved the problem:
First, set scrollingcontainer = "your scroll div DOM"
Then set up CSS
.ql-clipboard {
position: fixed !important;
left: 50% !important;
top: 50% !important;
}
try it

RobbinBaauw added a commit to RobbinBaauw/CSHub that referenced this issue Dec 29, 2019
RobbinBaauw added a commit to RobbinBaauw/CSHub that referenced this issue Dec 29, 2019
@Cobatkao
Copy link

@attacking do as you mentioned above, it really works! many thanks!

@fengchangfight
Copy link

this is how i fixed my problem:
before:
<vue-editor
@text-change="autosave"
:editorToolbar="customToolbar"
v-model="memDetail.content"
>

after:
<quill-editor
@change="autosave"
style="height: 480px"
v-model="memDetail.content"
:content="memDetail.content"
/>

I don't know why, but seems like a css problem

@Chris-Kin
Copy link

@attacking do as you mentioned above, it really works! many thanks!

Glad to help you

@quroom
Copy link

quroom commented Apr 2, 2021

this is how i fixed my problem:
before:
<vue-editor
@text-change="autosave"
:editorToolbar="customToolbar"
v-model="memDetail.content"

after:
<quill-editor
@change="autosave"
style="height: 480px"
v-model="memDetail.content"
:content="memDetail.content"
/>

I don't know why, but seems like a css problem

@change will make error. So scroll to top was not run in clipboard paste event.
If you have text style, It will be ignored.

@er-nabin-bhusal
Copy link

Following is my solution.
scrollingContainer="html, body"

.ql-clipboard {
    position: fixed;
    display: none;
    left: 50%;
    top: 50%;
  }


@sidraw27
Copy link

sidraw27 commented Jul 1, 2021

All you need to do was unset the outside container's overflow and change your container height.

.quillWrapper {
  overflow: unset !important;
  height: 500px !important;
}
.ql-container {
  height: 450px !important;
}

@alejoacevedor1
Copy link

In my case the editor is inside a different scrolling container, not 'html, body', which is why the above-mentioned fixes didn't exactly suit.

This simple workaround though did the job perfectly:

.ql-clipboard {
    position: fixed;
}

this work for me !!!

@quroom
Copy link

quroom commented Aug 30, 2021

I set scrollingContainer to html and add this css:

.ql-clipboard {
  position: fixed;
}

it fixed the problem for me. try it out.

This works me either.

@jasonhoo95
Copy link

Hi why when i undo the scrollbar wont scroll to the position where the text should be undo, with auto height, instead if i set a height for ql-editor it will scroll to the position when undo occurs?

@akshaykadambatt
Copy link

image

After pasting the curser lands on the beginning of the pasted text, is there any fix for that?

@janburchin
Copy link

janburchin commented Jan 30, 2023

I set scrollingContainer to html and add this css:

.ql-clipboard {
  position: fixed;
}

it fixed the problem for me. try it out.

For me, it did not work until I added !important to the css, so now it looks like this:

.ql-clipboard {
      position: fixed !important;
}

@indrajithvinodnair
Copy link

indrajithvinodnair commented Apr 4, 2023

This is the final fix which is inherited from the project's code itself.

Be sure to set the scrollingContainer to element that is responsible for scrolling the view. For example, let's say body element is responsible for scrolling the view, then we will have: scrollingContainer: html, body.

But sometimes body element is not responsible for scrolling the view, let's say we have an element around our Quill editor or anywhere inside of body element and it has a class of .editor-scroller, then we will have: scrollingContainer: .editor-scroller

import Quill, { RangeStatic, StringMap } from 'quill';
import Delta from 'quill-delta';

const ClipboardModule: any = Quill.import('modules/clipboard');

class CustomClipboard extends ClipboardModule {
  onPaste(event: ClipboardEvent): void {
    const range: RangeStatic = this.quill.getSelection();
    const formats: StringMap = this.quill.getFormat(range.index);
    const pastedDelta: Delta = this.convert(formats);
    const delta: Delta = new Delta()
      .retain(range.index)
      .delete(range.length)
      .concat(pastedDelta);
    this.quill.updateContents(delta, 'silent');
    // range.length contributes to delta.length()
    this.quill.setSelection(delta.length() - range.length, range.length, 'silent');
    this.quill.scrollIntoView();
  }
}

Quill.register('modules/clipboard', CustomClipboard, true);

The above code has been written in a TypeScript file, if you have a JavaScript file just remove type declarations.

For those who are going to write this in a TypeScript file, I used this library for Quill types: https://www.npmjs.com/package/@types/quill

This works. Thankyou

Edit: This works for the scroll jump issue, but introduces a new issue with respect to the styling of the pasted content. @ArsalanSavand

@belowsurface3000
Copy link

For me all problems with scroll jumps were gone, after I had set scrollingContainer: document.documentElement

and where did you set this?

@belowsurface3000
Copy link

I set scrollingContainer to html and add this css:

.ql-clipboard {
  position: fixed;
}

it fixed the problem for me. try it out.

For me, it did not work until I added !important to the css, so now it looks like this:

.ql-clipboard {
      position: fixed !important;
}

thanks a lot! this worked for me :)

@SHENGALTEC
Copy link

image

After pasting the curser lands on the beginning of the pasted text, is there any fix for that?

any fix for this issue yet?

@luin luin added the 1.x only Issues that have been addressed in v2 label Aug 1, 2023
@nomad-vagabond
Copy link

nomad-vagabond commented Aug 9, 2023

In my case setting 'position: fixed !important;' fixes the issue only in Firefox

.ql-clipboard {
      position: fixed !important;
}

But this setup by nabinbhusal80 works fine both in Firefox and Chromium:

.ql-clipboard {
    position: fixed !important;
    display: none;
    left: 50%;
    top: 50%;
  }

@syedmoazam
Copy link

@deepakjtrigent I found an issue when I implemented the below changes:

.ql-clipboard {
height: 100%;
width: 100%;
position: absolute;
left: 0;
top: 0;
z-index: -1;
}

If some text has been selected in the editor and we try to paste some text to replace with the selected text. Some of the text is missing when I pasted the new text. Depending on the number of characters in the old text.
Here is a video from https://quilljs.com/playground/

Quill.Issue.mp4

@shahabkamali
Copy link

this fixed my problem

var quill = new Quill('#editor', {
  modules: { toolbar: toolbarOptions },
    theme: 'snow',
    scrollingContainer: 'html'
});

@Amoyens1s
Copy link

this fixed my problem

var quill = new Quill('#editor', {
  modules: { toolbar: toolbarOptions },
    theme: 'snow',
    scrollingContainer: 'html'
});

wow! this works fine!!!! thanks!!

@quill-bot
Copy link

Quill 2.0 has been released (announcement post) with many changes and fixes. If this is still an issue please create a new issue after reviewing our updated Contributing guide 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x only Issues that have been addressed in v2 bug:functionality
Projects
None yet
Development

No branches or pull requests