Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wwedding’s picture

Patch tested here and it did the trick, no more terrible popup behavior.

idflood’s picture

I confirm the patch is working great and the code looks good. It was not applying anymore, so here is a simple reroll.

dboulet’s picture

Patch works for me too, thanks. Maybe we should add a small comment in the code to explain the changes here?

idflood’s picture

Here is the same patch as above with added comment. I've chosen to use a shortened version of the current issue title: fix broken popup browser when using "add another item"

LSU_JBob’s picture

Here's a patch so you're not hardcoding zero into the index, and putting a check to make sure the array is not empty just so the code can be bullet proof.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
582 bytes

Fixed just one whitespace issue, therefore I upload a new patch. Besides from that everything stays the same. I think we agree, that this could be commited.

dboulet’s picture

Fixed a few more spacing issues.

LSU_JBob’s picture

nice, thanks guys

dboulet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
567 bytes

Found one more space issue and simplified the code a little.

LSU_JBob’s picture

I still kinda like the if statements broken out, means it doesn't have to evaluate two, just one before anything else goes on. I think at this point though stuff like this is trvial

I think we should go with patch in #7

dboulet’s picture

LSU_JBob, the patch in #9 does the exact same thing. If the first condition for the if evaluates to false, that value will be returned and the second condition will be ignored. I see no reason to split it into two ifs.

idflood’s picture

The patch in #9 looks good. Just one suggestion maybe, why not use the jQuery.isArray() function? I think it makes the code more clear.

Here is a patch identical as #9, except that it uses the $.isArray function.
ref: http://api.jquery.com/jQuery.isArray/

idflood’s picture

One minute after I posted the last patch I thought about one more simplification. Since we assign options.widget.src to browserSrc we can use that variable for the condition and value assignment too.

dboulet’s picture

Nice. Now we just need the maintainers to take a look at it.

acbramley’s picture

Status: Needs review » Reviewed & tested by the community

Works for me. Let's get this committed!

minneapolisdan’s picture

Thanks for the patch, I used #13 and it seems to have fixed the issue.

rickvug’s picture

The patch in #13 is dead simple. Let's get it in as this bug will effect a lot of users.

rickvug’s picture

Status: Reviewed & tested by the community » Needs work

I tested the patch in #13 and it does not work for me. I'm using the Rubik theme, not sure if that would make a difference. I went back to the very first patch posted and had success with that.

idflood’s picture

@rickvug: what browser are you using?

rickvug’s picture

@idflood Chrome 13 for Mac.

idflood’s picture

I've tried to reproduce the issue described in #18 without luck. Tested on chrome 13 on osx, with first and last patch and they are both working. Tried again after insalling tao and rubick with rubick as the admin theme, and both patches are still working.

The important thing is that one of these patch get commited. Then it will be easier to fix edge cases.

@rickvug: Any idea of other modules / changes you made that may affect the result?

bforchhammer’s picture

Subscribe

ParisLiakos’s picture

Subscribing!will test the patch later

rickvug’s picture

@rootatwc @idflood Sorry that I haven't been able to get back to testing again. If you can't reproduce the problem feel free to commit the patch. If the problem crops up for me again I'll comment further.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community

Tested and works with both chrome and firefox in Linux!
As per #24,Please commit this!

ParisLiakos’s picture

Also tested it against 7.x-1.x.works there too!!
Can someone commit it on both branches?its an anoying bug:p

idflood’s picture

The issue still exists in 7.x-2.x. The patch in #13 still apply and fix it. please commit

Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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