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

Canvas pointer position is wrong when in a scrollable div #1821

Closed
cannontechnology opened this issue Nov 7, 2014 · 3 comments · Fixed by #2128
Closed

Canvas pointer position is wrong when in a scrollable div #1821

cannontechnology opened this issue Nov 7, 2014 · 3 comments · Fixed by #2128
Assignees
Labels
Milestone

Comments

@cannontechnology
Copy link

There is an issue when a canvas is placed in a scrollable div. When you scroll the pointer position is off by the amount scrolled.

The issue is with the scroll value in the getPointer fn below. If you remove the scroll.left and scroll.top values it works fine.

function getPointer(event, upperCanvasEl){
event || (event = fabric.window.event);
var element = event.target ||
(typeof event.srcElement !== unknown ? event.srcElement : null),
scroll = fabric.util.getScrollLeftTop(element, upperCanvasEl);
return {
x: pointerX(event) + scroll.left,
y: pointerY(event) + scroll.top
};
}

Please see the following fiddle:
http://jsfiddle.net/mcannon/6sksLp30/3/

Thanks,

Matt

@asturur
Copy link
Member

asturur commented Apr 19, 2015

This updated fiddle, with overriden function getScrolTopLeft solves the problem.
http://jsfiddle.net/6sksLp30/10/

I do not have time to check if it creates other.
I do not understand why an absolute position element should not be checked for scrollTop and scrollLeft.
I just changed from:
https://github.com/kangax/fabric.js/blob/master/src/util/dom_misc.js#L124

      if (element.nodeType === 1 &&
          origElement !== upperCanvasEl &&
          fabric.util.getElementStyle(element, 'position') === 'absolute') {
-        left = 0;
-        top = 0;
+        left += element.scrollLeft || 0;
+        top += element.scrollTop || 0;
      }

@kangax any idea why is not considered?

@asturur asturur added the bug label Apr 19, 2015
@kangax kangax added this to the 1.5.1 milestone May 6, 2015
@asturur
Copy link
Member

asturur commented Jun 8, 2015

http://jsfiddle.net/asturur/7fsw81mp/5/

I manage to reduce the fiddle to a minimum amount of code and html markup that still has the problem.

I added calcOffset just to be sure that is not simple auto fixing.

this is the commit that added:

      if (element.nodeType === 1 &&
          origElement !== upperCanvasEl &&
          fabric.util.getElementStyle(element, 'position') === 'absolute') {
        left = 0;
        top = 0;
      }

ddaf8ba

I checked the issue but is very generic and i cannot find the reason of absolute position resetting the temp vars to 0.

@Kienz do you remember something?

@Kienz
Copy link
Collaborator

Kienz commented Jun 8, 2015

@asturur I think I remember. If you resize the object and while resizing moving the mouse over the scrollbar of the layer-div, the size of the objects jumps.
http://jsfiddle.net/r2ZE7/299/

But If you remove this if statement it works the same way. I think we can drop this.
Another issue exists with freedrawing. If you draw a line and moving the mouse over the scrolled scrollbar, the freedrawing line jumps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants