Sigh

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

This 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...

aspilicious’s picture

Isn't solution very easy?

When loading a page, always remove old link, and then add a new one.

casey’s picture

Oops. 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.

aspilicious’s picture

Status: Active » Needs review
FileSize
803 bytes

THIS 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

aspilicious’s picture

This is a better place (peaking at #3)

casey’s picture

Status: Needs review » Needs work

You also need to remove tabs twice.

aspilicious’s picture

Another try

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

Needzzz a review cause itzz green and im prety sure itzzz working.

casey’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

ksenzee’s picture

I'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.

aspilicious’s picture

The 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.

ksenzee’s picture

Yes, 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.

dww’s picture

I 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! ;)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

YAY!!! Thank you all SO much! :D

Committed to HEAD!

Kiphaas7’s picture

ksenzee, 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.)

Status: Fixed » Closed (fixed)
Issue tags: -webchick's D7 alpha hit list

Automatically closed -- issue fixed for 2 weeks with no activity.