Skip to content

Sortable - fixed parentOffset calculation #1093

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Sortable - fixed parentOffset calculation #1093

wants to merge 2 commits into from

Conversation

adamziel
Copy link

In Sortable plugin in the _getParentOffset method there is a bit of code described as "a special case where we need to modify a offset calculated on start" which adds scrollParent.scrollTop() to the parentOffset.top.

It does it if (among others) scrollParent is not equal to document. On chrome the condition was evaluating to true even if scrollParent was document.body; This pull request adds necessary check for document.body.

Fixed calculation of "a special case where we need to modify a offset…
cb07a80 
… calculated on start"

It was altering parentOffset under wrong condition - scrollParent was compared to document instead of document.body
@scottgonzalez
Copy link
Member

Before we can review this, we'll need you to sign the CLA, file a ticket on http://bugs.jqueryui.com with a reduced test case, and add unit tests showing that the problem is fixed.

@adamziel
Copy link
Author

adamziel commented Oct 4, 2013

Okay I did as you asked, also here's a related ticket: http://bugs.jqueryui.com/ticket/9588
I will add a test case when I figure if and how this particular issue may be unit-tested

gnarf
gnarf reviewed Oct 8, 2013
View reviewed changes
ui/jquery.ui.sortable.js
@@ -954,7 +954,7 @@ $.widget("ui.sortable", $.ui.mouse, {
// 1. The position of the helper is absolute, so it's position is calculated based on the next positioned parent
// 2. The actual offset parent is a child of the scroll parent, and the scroll parent isn't the document, which means that
// the scroll is included in the initial calculation of the offset of the parent, and never recalculated upon drag
if(this.cssPosition === "absolute" && this.scrollParent[0] !== document && $.contains(this.scrollParent[0], this.offsetParent[0])) {
if(this.cssPosition === "absolute" && this.scrollParent[0] !== document && this.scrollParent[0] !== document.body && $.contains(this.scrollParent[0], this.offsetParent[0])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the description of this issue in the comment block that describes the if that you modified?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this pull request

@jzaefferer
Copy link
Member

@azielinski the ticket describes how to reproduce the issue, have you tried to pour that into a unit test? Currently I can't tell where this got stuck.

@adamziel
Copy link
Author

@jzaefferer Sure I can do that, can you just point me in the right direction? This bug is related to overflow-x:hidden on body combined with Y scrolling, and I cannot do that in tests/unit/sortable/sortable.html since a) overflow-x:hidden on body would affect other tests b) there is no scrolling since #qunit-fixture receives position:absolute and top:-10000px

@mikesherov
Copy link
Member

@azielinski thanks for contributing, but as there's been no movement on this for quite a while, I'm going to close this. Please let me know if you'd like to try again on this.

@mikesherov mikesherov closed this Aug 11, 2014
@Arkh1
Copy link

Arkh1 commented Jan 8, 2015

Just as a heads up... This is still an issue. I ran into the same problem and patching with AZielinski's suggested change fixed the issue.

@dmethvin
Copy link
Member

dmethvin commented Jan 8, 2015

So the comment @azielinski added in 3dbdfee clarifies it is trying to fix this Chrome bug. @mikesherov has been prodding the Chrome team in that ticket to land the fix.

I have a feeling this same problem may be relevant to my interests because of jquery/jquery#1714, where the PR proposes to add offsetParent.scrollTop() to the offset, but probably shouldn't if the offsetParent is body (Chrome) or document (everyone else).

@mikesherov
Copy link
Member

@Arkh1 the proper fix for this calculation is already in the draggable widget. I just haven't yet gotten around to doing all the necessary cleanup and testing for the same thing in sortable yet. There are a ton of edge cases though. :-\

@Mykybo
Copy link

Mykybo commented Jun 17, 2016 •
edited
Loading

Can this be fixed here please? Or at least provide a way how to workaround it. Waiting for Chrome to fix its behavior will surely take much longer (look how long it is since this was first reported). To me it seems quite common that libraries like jQuery includes fixes for browser long-living bugs.

http://jsbin.com/fejepocape/1/edit?html,js,output - just scroll down and try to drag any item

@blueforesticarus
Copy link

I dont understand why the initial pull request was closed. He provided a fix for the bug, and it was rejected because it didn't have a unit test? It would seem you are allowing a known bug to remain because of the possiblity that some other more obscure bug may still exist. Unit tests aren't all that helpful if you already know the software doesn't work.

bnf added a commit to bnf/typo3 that referenced this pull request Dec 11, 2021
@bnf
[TASK] Reconstruct patch file for jquery-ui/ui/sortable.js
96518ce 
jquery-ui/sortable has been patched as minified file in #83593,
without any sources available (only a related pull requests).
That means neither updates, nor a review of the bundled
code is possible.

The patch is now derived from the original pull request
jquery/jquery-ui#1093
and adapted to incorporate the "support for iframes"
BUGFIX from https://bugs.jqueryui.com/ticket/9604 – see
jquery/jquery-ui@fa460f36dc819
(in order to be applicable to our currently installed
jquery-ui version)

Executed commands:

  curl -s https://patch-diff.githubusercontent.com/raw/jquery/jquery-ui/pull/1093.diff | \
    sed -e 's:/ui/jquery.ui.sortable.js:/node_modules/jquery-ui/ui/sortable.js:g' \
        -e 's/954/1001/g' \
        -e 's/scrollParent\[0\] !== document/scrollParent[0] !== this.document[0]/' \
    > patches/jquery-ui+1.11.4.patch

  (cd Build; grunt build)

Resolves: #96335
Related: #83593
Related: #96323
Releases: main
Change-Id: I71360f2808c655a0af00259552dcf8507742b729
bnf added a commit to bnf/typo3 that referenced this pull request Dec 14, 2021
@bnf
[TASK] Reconstruct patch file for jquery-ui/ui/sortable.js
bcc763e 
jquery-ui/sortable has been patched as minified file in #83593,
without any sources available (only a related pull requests).
That means neither updates, nor a review of the bundled
code is possible.

The patch is now derived from the original pull request
jquery/jquery-ui#1093
and adapted to incorporate the "support for iframes"
BUGFIX from https://bugs.jqueryui.com/ticket/9604 – see
jquery/jquery-ui@fa460f36dc819
(in order to be applicable to our currently installed
jquery-ui version)

Executed commands:

  curl -s https://patch-diff.githubusercontent.com/raw/jquery/jquery-ui/pull/1093.diff | \
    sed -e 's:/ui/jquery.ui.sortable.js:/node_modules/jquery-ui/ui/sortable.js:g' \
        -e 's/954/1001/g' \
        -e 's/scrollParent\[0\] !== document/scrollParent[0] !== this.document[0]/' \
    > patches/jquery-ui+1.11.4.patch

  (cd Build; grunt build)

Resolves: #96335
Related: #83593
Related: #96323
Releases: main
Change-Id: I71360f2808c655a0af00259552dcf8507742b729
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Dec 14, 2021
@bnf @ohader
[TASK] Unpatch jquery-ui/ui/sortable.js
777d637 
jquery-ui/sortable has been patched as minified file in #83593
with a patch taken from jquery/jquery-ui#1093

That pull request never got merged, but in the meantime the
chrome bug seems to be fixed (the original report is no longer
reproducable), therefore we now remove the patch.

Note: Our binary patch (#83593) implicitly reverted jquery-ui/sortable
to a 2013er version. Thus, this change implicitly updated our copy
back to version 1.11.4 (from 2015) as defined in our package.json

Resolves: #96335
Related: #83593
Related: #96323
Releases: main
Change-Id: I71360f2808c655a0af00259552dcf8507742b729
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/72628
Tested-by: core-ci <typo3@b13.com>
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Dec 14, 2021
@bnf @ohader
[TASK] Unpatch jquery-ui/ui/sortable.js
0cfdaa5 
jquery-ui/sortable has been patched as minified file in #83593
with a patch taken from jquery/jquery-ui#1093

That pull request never got merged, but in the meantime the
chrome bug seems to be fixed (the original report is no longer
reproducable), therefore we now remove the patch.

Note: Our binary patch (#83593) implicitly reverted jquery-ui/sortable
to a 2013er version. Thus, this change implicitly updated our copy
back to version 1.11.4 (from 2015) as defined in our package.json

Resolves: #96335
Related: #83593
Related: #96323
Releases: main
Change-Id: I71360f2808c655a0af00259552dcf8507742b729
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/72628
Tested-by: core-ci <typo3@b13.com>
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@adamziel @scottgonzalez @jzaefferer @mikesherov @Arkh1 @dmethvin @Mykybo @blueforesticarus @gnarf