Skip to content

Conversation

@mehranshakeri
Copy link
Contributor

Using jQuery contextMenu with d3 tree (http://bl.ocks.org/mbostock/4339083) object creates a problem with positioning the menu in wrong place.

ContextMenu in opening time checks "offsetX" attribute of event and in case if exists, it does some calculation to position the menu. But in combination with d3 tree, this value will be calculated wrong and menu will be positioned in far right side of page. I have seen in absence of "offsetX" attribute, "pageX" is used. I could fix this issue with replacing the whole condition block with just else part.
Is there any reason for using offsetX when pageX exists all the time?

@anseki
Copy link
Contributor

anseki commented Mar 7, 2016

It is positioned wrong, in pages without D3 also.
See this:
https://jsfiddle.net/fhmf00oy/

In Chrome, I right clicked "TRIGGER" and I didn't move mouse:

s01

offsetX/Y still have compatibility problems.
See this:
https://jsfiddle.net/ekbj8asf/

Firefox:
s02

Chrome:
s03

It seems Chrome gets those of inline-element by content-box-based, and Firefox gets those by border-box-based.

As @mehranshakeri said, I also think that pageX/Y should be used.

@anseki
Copy link
Contributor

anseki commented Mar 7, 2016

This issue occurs in IE also.

@bbrala
Copy link
Member

bbrala commented Mar 13, 2016

Thanks for the contribution. I'll have a look at it soon. I'll try and find out why there is a offsetX used so we might be able to remove it. The pageX fallback when there is no offsetX has been added only this januari, so i'll dig around and see if i can see a valid reason to keep it.

@bbrala bbrala added the Bug label Mar 13, 2016
@AllBogs
Copy link

AllBogs commented Apr 20, 2016

Howdy. I seem to be running into the same issue in a project. As a workaround, I have reverted to version 2.0.1 for the time being.

@psolom
Copy link

psolom commented Apr 26, 2016

Same thing!
It worked fine for me untill I added div wrapper to the page to center a content as:

<body>
    <div class ="wrapper">
        Content here...
    <div>
</body>
.wrapper {
    width: 1200px;
    margin: 0 auto;
}

I have reverted to 2.0.1 as @AllBogs did, and it works correct.
Fix please.

@bbrala
Copy link
Member

bbrala commented Apr 26, 2016

Hi guys,

I was looking into this and my conclusion is the earlier "fix" that added offsetX should be reverted sadly. I'll try and accept these changes this week and release a new version so you update again.

Thanks all for the input.

@anseki
Copy link
Contributor

anseki commented Apr 26, 2016

At least, @mehranshakeri's patch seems to working fine.

@bbrala
Copy link
Member

bbrala commented Apr 26, 2016

Yeah his patch is basically remove the offsetX/Y from the code again :)

@bbrala
Copy link
Member

bbrala commented Apr 29, 2016

Note to self; original PR #354

psolom added a commit to psolom/Filemanager that referenced this pull request May 7, 2016
- implemented AWS S3 plugin
- extended list of previewed file types via ViewerJS; @see config["pdfs"]["pdfsExt"]
- added preview of MS Office file types via Google Docs Viewer; @see config["docs"]["showGoogleViewer"]
- Dropzone file uploader replaced with Blueimp's jQuery-File-Upload, which provides out of the box following features:
	- chunked upload; @see config["upload"]["chunkSize"]
	- image resizing; Wideimage lib was removed,
	- image orientation (orientation), etc.
- "upload" section in config file was
- added new "Type" column in list view;
- new "downloadItem" method, to handle download errors correctly;
- better error handling for client-size and in PHP connector;
- jQuery-ContextMenu plugin reverted to v.2.0.1 until the bug with positioning is fixed; @see swisnl/jQuery-contextMenu#354
- IMPORTANT: PHP is the only actual connector; compatibility with others connectors was completely broken, refactoring required
@bbrala bbrala merged commit 5cdb0a9 into swisnl:master May 24, 2016
@bbrala
Copy link
Member

bbrala commented May 24, 2016

I merged this to fix the issue for the people having it.

#341 needs to be revisited.

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 this pull request may close these issues.

5 participants