Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When I use multiple field, that has "Add another item" button, browser popup breaks if using that button. Attached patch fixes it.
Comment | File | Size | Author |
---|---|---|---|
#13 | multiple_mediafield_toolbar-1228790-13.patch | 525 bytes | idflood |
#12 | multiple_mediafield_toolbar-1228790-12.patch | 557 bytes | idflood |
#9 | multiple_mediafield_toolbar-1228790-9.patch | 567 bytes | dboulet |
#7 | multiple_mediafield_toolbar-1228790-7.patch | 587 bytes | dboulet |
#6 | multiple-mediafield-toolbar-1228790.patch | 582 bytes | slashrsm |
Comments
Comment #1
wwedding CreditAttribution: wwedding commentedPatch tested here and it did the trick, no more terrible popup behavior.
Comment #2
idflood CreditAttribution: idflood commentedI confirm the patch is working great and the code looks good. It was not applying anymore, so here is a simple reroll.
Comment #3
dboulet CreditAttribution: dboulet commentedPatch works for me too, thanks. Maybe we should add a small comment in the code to explain the changes here?
Comment #4
idflood CreditAttribution: idflood commentedHere 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"
Comment #5
LSU_JBob CreditAttribution: LSU_JBob commentedHere'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.
Comment #6
slashrsm CreditAttribution: slashrsm commentedFixed 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.
Comment #7
dboulet CreditAttribution: dboulet commentedFixed a few more spacing issues.
Comment #8
LSU_JBob CreditAttribution: LSU_JBob commentednice, thanks guys
Comment #9
dboulet CreditAttribution: dboulet commentedFound one more space issue and simplified the code a little.
Comment #10
LSU_JBob CreditAttribution: LSU_JBob commentedI 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
Comment #11
dboulet CreditAttribution: dboulet commentedLSU_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.
Comment #12
idflood CreditAttribution: idflood commentedThe 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/
Comment #13
idflood CreditAttribution: idflood commentedOne 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.
Comment #14
dboulet CreditAttribution: dboulet commentedNice. Now we just need the maintainers to take a look at it.
Comment #15
acbramley CreditAttribution: acbramley commentedWorks for me. Let's get this committed!
Comment #16
minneapolisdan CreditAttribution: minneapolisdan commentedThanks for the patch, I used #13 and it seems to have fixed the issue.
Comment #17
rickvug CreditAttribution: rickvug commentedThe patch in #13 is dead simple. Let's get it in as this bug will effect a lot of users.
Comment #18
rickvug CreditAttribution: rickvug commentedI 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.
Comment #19
idflood CreditAttribution: idflood commented@rickvug: what browser are you using?
Comment #20
rickvug CreditAttribution: rickvug commented@idflood Chrome 13 for Mac.
Comment #21
idflood CreditAttribution: idflood commentedI'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?
Comment #22
bforchhammer CreditAttribution: bforchhammer commentedSubscribe
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedSubscribing!will test the patch later
Comment #24
rickvug CreditAttribution: rickvug commented@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.
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedTested and works with both chrome and firefox in Linux!
As per #24,Please commit this!
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedAlso tested it against 7.x-1.x.works there too!!
Can someone commit it on both branches?its an anoying bug:p
Comment #27
idflood CreditAttribution: idflood commentedThe issue still exists in 7.x-2.x. The patch in #13 still apply and fix it. please commit
Comment #28
Dave ReidCommitted #13 to both branches. Thanks!
http://drupalcode.org/project/media.git/commit/1748396
http://drupalcode.org/project/media.git/commit/1d9bdfe