I'm not sure which module this belongs under, but I'm going to start with Overlay.
Steps to reproduce:
1. Go to admin/structure/types/manage/article/fields. There is one "add to shortcut" link.
2. Add a new field.
3. Get redirected to admin/structure/types/manage/article/fields/field_dssdf/field-settings%3Fdestinations%5B0%5D%3Dadmin%2Fstructure%2Ftypes%2Fmanage%2Farticle%2Ffields%2Ffield_dssdf%2Fedit%26destinations%5B1%5D%3Dadmin%2Fstructure%2Ftypes%2Fmanage%2Farticle%2Ffields%253Frender%253Doverlay%26render%3Doverlay (really?) and see that there are now two add-to-shortcut links.
4. Save those field settings and get directed to admin/structure/types/manage/article/fields/field_dssdf/edit%3Fdestinations%5B0%5D%3Dadmin%2Fstructure%2Ftypes%2Fmanage%2Farticle%2Ffields%253Frender%253Doverlay%26render%3Doverlay where there are now three of them.
Comment | File | Size | Author |
---|---|---|---|
#7 | multiple_shortcut_links_proper_fix_8.patch | 1.17 KB | aspilicious |
#5 | multiple_shortcut_links_proper_fix_6.patch | 857 bytes | aspilicious |
#4 | multiple_shortcut_links.patch | 803 bytes | aspilicious |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedThis happens every time you reload a page. I think we just have to check if we are reloading, or check if we alrdy have a button...
Comment #2
aspilicious CreditAttribution: aspilicious commentedIsn't solution very easy?
When loading a page, always remove old link, and then add a new one.
Comment #3
casey CreditAttribution: casey commentedOops. Related to #674852: Remove shortcut-add-remove-link and tabs in Drupal.overlay.load instead of Drupal.overlay.bindChild. Iframe can be reload/redirected in which case overlay.load() isn't called.
We need those two lines at both places.
Comment #4
aspilicious CreditAttribution: aspilicious commentedTHIS IS PROOVE OF CONCEPT DONT BASH ME
Ok, I made a patch that solves the issue. I just coppied the remove shortcut lines and put them in bind.child function.
Probably this is insane, and wrong and whateva, but at least the bug is fixed ;)
So js people tell me were the right place is for these lines.
EDIT: after reading #3 i was prety close
Comment #5
aspilicious CreditAttribution: aspilicious commentedThis is a better place (peaking at #3)
Comment #6
casey CreditAttribution: casey commentedYou also need to remove tabs twice.
Comment #7
aspilicious CreditAttribution: aspilicious commentedAnother try
Comment #8
aspilicious CreditAttribution: aspilicious commentedComment #9
aspilicious CreditAttribution: aspilicious commentedNeedzzz a review cause itzz green and im prety sure itzzz working.
Comment #10
casey CreditAttribution: casey commentedThis means those two lines are called twice; in load() and bindChild. But I can't think of another solution.
This obviously needs to be fixed so patch is ok by me.
Comment #11
ksenzeeI'm okay with this also.
I have a related concern that I don't really know where to bring up, so I'll mention it here. We need to do way more manual functional testing of overlay patches. This issue wouldn't exist if there had been more "click around and see what breaks" testing of #674852: Remove shortcut-add-remove-link and tabs in Drupal.overlay.load instead of Drupal.overlay.bindChild. Mind you, I was zero help on that issue -- I didn't even see it before commit -- so I'm not criticizing at all. I'm just saying we need to make a concerted effort to break patches before deciding they're RTBC. Enable a module, add a shortcut, remove it, remove an existing shortcut, reload, close with ESC, click admin links, click toolbar links, click regular links, run batch operations (when that's working again!), create content, upload images, delete content, make a poll and vote on it, the whole nine yards. Oh, and in IE, FF, and Safari/Chrome.
To be honest, I haven't looked at #237566: Automated JavaScript unit testing framework closely enough to know whether it would handle this kind of functional testing, or if we need a Selenium suite. But until we have something automated, we *are* the JS testbot.
Comment #12
aspilicious CreditAttribution: aspilicious commentedThe other topic was open for 6 days!!!! So you could easily tested it. And i had the patch running on my computer for 3 days and i didn't noticed the problems. I did some stuff but not everything...
I will try to test better.
Comment #13
ksenzeeYes, I could have tested it, and didn't, which is what I meant when I said that I was no help whatsoever, and why I specifically said I'm not criticizing anyone. The whole situation just makes me nervous. We have this overarching system enabled by default, with no tests whatsoever, and a culture that's quickly gotten used to automated testing. ("It's green, commit it!") Maybe I should just shut up and write some Selenium tests.
Comment #14
dwwI ran into this while testing #649224: Cannot run certain batch processes (tests, update manager, etc) with the overlay enabled. I can confirm that patch #7 fixes the bug. I can't speak to the underlying JS code and the logic of the patch -- overlay JS is out of my league. But, if ksenzee and others are happy, ship it! ;)
Comment #15
webchickYAY!!! Thank you all SO much! :D
Committed to HEAD!
Comment #16
Kiphaas7 CreditAttribution: Kiphaas7 commentedksenzee, you have a point, but then again, there are so many options to test against, and with the primary focus usually laying on the case that the bug wants to fix, it's easy to miss one or two.
(Probably the very same reason why core now has automated testing, and why there is such a need for automated javascript and performance testing, missing the obvious testing points.)