Dragging and dropping a shortcut from disabled to enabled doesn't switch the status if the dragging happens fast [#1386514]

From comments in #1344752: Drag and Drop of shortcuts between Enable and Disable Does not respect the Shortcuts Limit (note that this bug is probably not reproducible until after that patch is committed; we decide to postpone it to this followup issue instead):

David_Rothstein (comment #17):

I found that if I drag something from "disabled" to a place near the bottom of the enabled list, it works as intended. But if I drag from "disabled" all the way to the top (or close to the top), the swap doesn't happen correctly and you still wind up with 8 enabled shortcuts after you save (even though you only see 7 on the screen before you save).

pflame (comment #19):

I am able to reproduce the bug. It happened when I drag items from disabled to enabled section very fast. I am working on it.

pflame (comment #21):

The bug is bit complicated. It seems the problem is with tabledrag.js. I am still finding efficient way to solve the problem.

We can start this issue in the Shortcut module queue for now but move it to a tabledrag.js bug if it turns out to be necessary.

Comment	File	Size	Author
#10	shortcuts-1386514-10.patch	2.19 KB	David_Rothstein
#10
#10	interdiff-8-10.txt	833 bytes	David_Rothstein
#8	shortcuts-1386514-8.patch	2.19 KB	pflame
#8
#5	shortcuts-1386514-5.patch	2.28 KB	nod_
#5
#3	shortcuts-1386514-3.patch	1.73 KB	pflame
#3
#3	shortcuts-1386514-3.patch	1.73 KB	pflame
#3
#1	shortcuts-1386514-1.patch	1.48 KB	pflame
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

pflame CreditAttribution: pflame commented 11 January 2012 at 12:06

Status:

Active

» Needs review

File	Size
shortcuts-1386514-1.patch	1.48 KB

David_Rothstein,

I could able to fix this bug without touching tabledrag.js. Check the attached patch file.

The swappedRow variable that is passed by tabledrag.js to onswap event handler, is not the actual swapped element here. Because here, swapping happens also based on shortcut links limit.

-          disabled.after(disabled.prevAll().filter(':not(.shortcut-slot-empty)').get(0));
-          if ($(swappedRow).hasClass('draggable')) {
+          changedRow = disabled.prevAll().filter(':not(.shortcut-slot-empty)').get(0);
+          disabled.after(changedRow);
+          if ($(changedRow).hasClass('draggable')) {

From the first line of above code, we can notice that, the last element in the enabled selection will be moved as first element to the disabled section. So we need to mark this element as changed element. When I drag and drop slowly this element(changedRow variable) and the swappedRow variable point to same dom element, but if I drag and drop fast they are different.

Comment #2

David_Rothstein CreditAttribution: David_Rothstein commented 13 January 2012 at 05:49

Huh! Nice work. I think you are right.

It would be great to get a code comment explaining the difference between swappedRow and changedRow (so that someone reading the code can understand why one needs to be used rather than the other).

Comment #3

pflame CreditAttribution: pflame commented 15 January 2012 at 13:00

File	Size
shortcuts-1386514-3.patch	1.73 KB

shortcuts-1386514-3.patch	1.73 KB

David_Rothstein,

Thanks for the review. Added comments explaining the changedRow. Please check the attached patch.

I have also fixed issue Could Not Drag shortcut link from Disabled Section to Enabled section through Keyboard, but the fix is in the line below, which is changed in this bug. How do I provide patch to this bug. Shell I wait until this patch is committed?

 changedRow = disabled.prevAll().filter(':not(.shortcut-slot-empty)').get(0);

Need to change the above line to

 changedRow = disabled.prevAll().not($(this.element)).filter(':not(.shortcut-slot-empty)').get(0);

Comment #4

pflame CreditAttribution: pflame commented 15 January 2012 at 13:01

sorry by mistake attached same patch 2 times in the above comment.

Comment #5

nod_

French

Lille

CreditAttribution: nod_ commented 16 January 2012 at 10:43

File	Size
shortcuts-1386514-5.patch	2.28 KB

Please be careful of not introducing global variables by accident. prevAll() can filter the results, no need for the filter() right after it.

The file could use some cleanup too.

Comment #6

nod_

French

Lille

CreditAttribution: nod_ commented 16 January 2012 at 10:46

Issue tags:

+JavaScript

forgot tag

Comment #7

David_Rothstein CreditAttribution: David_Rothstein commented 17 January 2012 at 04:09

Good catch on the global variables, etc.

I think the patch looks good now, but the code comment still needs a little work. The spacing isn't correct (there should be as many characters on each line as can fit, but not going over 80 characters), and the capitalization isn't quite right... Also, I don't think the second sentence adds much that wasn't in the first?

I'd suggest rewriting the first sentence to say "To maintain the shortcut links limit, we need to move the last element from the enabled section to the disabled section." Then, if it's possible to come up with a second sentence to specifically explain why swappedRow is the wrong thing to use here (i.e., what is the difference between swappedRow and changedRow?), let's add that... If not, the one sentence is probably OK.

I have also fixed issue Could Not Drag shortcut link from Disabled Section to Enabled section through Keyboard, but the fix is in the line below, which is changed in this bug. How do I provide patch to this bug. Shell I wait until this patch is committed?

Nice! I would say either wait until this is committed, or, if it's really just that small of a change on top of the current patch, feel free to roll it into this patch and we could just fix both at once. It's up to you.

If you decide to include it here, you can close the other issue as a duplicate of this one.

Comment #8

pflame CreditAttribution: pflame commented 21 January 2012 at 07:37

File	Size
shortcuts-1386514-8.patch	2.19 KB

Hi David_Rothstein,

Thanks for the review. I changed the first line of comment as you suggested. I felt that, if we keep a comment about why swappedRow can not be used, it will be confusing. So I just put 1 sentence and removed the second sentence.

I included the fix to Could Not Drag shortcut link from Disabled Section to Enabled section through Keyboard in this patch and I close other issue as duplicate.

Please check the new patch.

Comment #9

nod_

French

Lille

CreditAttribution: nod_ commented 24 January 2012 at 00:14

Status:

Needs review

» Reviewed & tested by the community

Works fine for me, keyboard and mouse.

Just need to know if David_Rothstein is ok with the comments and I guess it's good to go.

Comment #10

David_Rothstein CreditAttribution: David_Rothstein commented 24 January 2012 at 05:07

File	Size
interdiff-8-10.txt	833 bytes
shortcuts-1386514-10.patch	2.19 KB

Yup, looks good to me, and it works when I tested it also.

I did a quick reroll to make the comment wrap at 80 characters (per coding standards, http://drupal.org/node/1354) and am attaching that, but it should be ready to go.

Thanks!