If either the transcoder or the player used in the atom reprensentation need some JavaScript to behave properly, then it doesn't work when used in an atom reference drop zone. I guess the easiest way to reproduce using only contrib module is through the Picture module ; use a picture group as a transcoder, and try to drop an image atom in a drop zone using a browser without native <picture /> support (pretty much everything except Chrome right now). Without the behaviors, the polyfill won't kick in.

Excepted result: Have the image displayed in the drop zone
Actual result: Depending on the browser, the atom caption that Picture puts in the alt tag is displayed, either in a "broken image box" or as plain text.

That used to work, it was fixed in 665de6d8, which added a Drupal.attachBehaviors(this);. That part was removed in cf60c70 ; there's a bunch of change in there that looks wrong. I think the patch wasn't correctly merged for the conflicts that arose after 665de6d8 landed, and the final patch commited in cf60c70 ended up reverting a bunch of good changes. I'm pretty sure removing that attachBehaviors wasn't supposed to happen, and reverting the part dealing with Drupal.dnd.currentAtom seems dubious too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nagy.balint’s picture

Thanks for the report, added it to the meta task to be fixed before next release.

nagy.balint’s picture

Status: Active » Needs work
FileSize
2.07 KB

First patch, needs more test work.

DeFr’s picture

This is still missing a e.originalEvent.dataTransfer.getData('Text') → Drupal.dnd.currentAtom ; that being said, I agree that ressource_id is a nicer variable name than dt, so let's keep the new variable name ;-)

nagy.balint’s picture

Okey, done. I also renamed it to resource_id, as it should be only with a single 's' in english :)

nagy.balint’s picture

My problem is that on my firefox 37, it does not matter if i have that attach there or not, the image always appear (the behavior does not work though, in the sense that the image does not change size when i reduce the size like on chrome does, but the image appears).

So having a hard time validating the patch.

DeFr’s picture

Do you have the experimental picture support enabled in Firefox through about:config ? (dom.image.picture.enabled and / or dom.image.srcset.enabled)

DeFr’s picture

Another wild guess: might also be due to using Picture 1.x. In Picture 1, the fallback image add a real src attribute, meaning that even without the polyfill, you'd see a standard image showing up. They changed the implementation in Picture 2, to make the fallback image only have an srcset attribute ; having only an srcset prevents double loading of the picture for browsers without native support (the browser see the img tag, starts downloading, and then the polyfill kicks in through JS, meaning some picture were download two times), but it also means that without the behavior, you don't get any image showing up.

nagy.balint’s picture

Hmm, i have picture 7.x-2.9+3-dev, and both options are false on my firefox.

Yet, even if i comment that attach line in the patch, i still get the image displayed. But its because my image inside the picture tag has both an src and a srcset attribute.

nagy.balint’s picture

Ah i got it.. there is a second Drupal.dnd.fetchAtom('', resource_id, function() { right after there which has an attach behaviors at the end. So actually we already had the attach there.
Of course its a good question why we have 2 fetchAtoms...

nagy.balint’s picture

Ah i see... so the thing is that the logic is like if the atom is already in the javascript settings, then it wont load it again.
Which is fine, but actually it does not have the permissions related to the atom, so in order to get the view and edit permission for the atom, it has to do a second fetchAtom, but with empty context parameter, which will force a reload.

At any rate, then the attach was already there at the end.

nagy.balint’s picture

Status: Needs work » Needs review
FileSize
5.33 KB

So this patch seems to work for me. I get the image perfectly fine now in firefox. And if i remove the attach at the end in the fetchatom with the empty context parameter then the image does not appear anymore like you explained. So it seems its fine like this.

nagy.balint’s picture

Removed the double fetchAtom at the drop function by adding the actions to the initial library atom settings.

This way the code makes more sense.

Please check if it works for you as well.

nagy.balint’s picture

FileSize
7.83 KB

1 more thing.
The atom reference refresh when coming back from the edit form (after clicking on the new edit link) has to be updated as well, to reapply the behavior, and to not cause an infinite cycle of atom fetches :)

Tested under firefox 37, ie9, and chrome, seems to work fine.

gifad’s picture

#11 did not work, but #12 and #13 are OK.

Tested with Firefox 37 and Safari 6 on MacOSX…

DeFr’s picture

@nagy.balint in #10-11: There's something fishy somewhere, because in current dev there's no call to attachBehaviors without context parameter. There's an attachBehaviors, but, it's context is limited to the operations button, and that's the root cause of this issue. (Should maybe have been clearer in the OP that the root cause was the replacement of Drupal.attachBehavriors($this); by Drupal.attachBehaviors($operation_buttons); )

That being said, #12 and #13 bring back the behaviors attachment on the whole drop zone, and thus works.

Overall, #13 seems like a really nice atom reference cleanup; getting rid of the double fetching is cool.

Given that this issue is changing a lot of line anyway due to the variable name fixup, it might also be good to get rid of atom reference custom parsing of the SAS that was there since 6.x, and use Drupal.dnd.sas2array introduced in 7.x ; the SAS format is unlikely to undergo massive change at that point, but, the less regexp matching, the better. (Concretly, that means replacing Drupal.dnd.currentAtom.replace('[scald… by Drupal.dnd.sas2array(Drupal.dnd.currentAtom).sid )

nagy.balint’s picture

FileSize
7.82 KB

For some reason the image appeared for me even with only applying the behaviors on the buttons, but maybe it only worked partially or something else was behind it :)

But anyways, now its bulletproof :)

Updated patch with the sas2array improvement.

nagy.balint’s picture

Status: Needs review » Fixed

Thanks! Committed.

  • nagy.balint committed 07428b7 on 7.x-1.x
    Issue #2469015 by nagy.balint, DeFr, gifad: Atom Reference: JS behaviors...

Status: Fixed » Closed (fixed)

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