Problem/Motivation

The Drupal.behaviors.fluidModal will attach it events every time behaviours are run on the page. Among other things, this will produce multiple callbacks to Drupal.entityBrowserModal.fluidDialog.

Proposed resolution

Attach Drupal.behaviors.fluidModal events only ones per document/iframe.

CommentFileSizeAuthor
#10 with patch applied.png13.89 KBoknate
#9 multiple firings.png14.39 KBoknate
#6 entity-browser-behaviours-run-once-3055370-6.patch982 bytespivica
#4 actual entity browser display with the patch.png275.28 KBsasanikolic
#4 expected entity browser display.png541.36 KBsasanikolic
#2 entity-browser-behaviours-run-once-3055370-2.patch1.64 KBpivica
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pivica created an issue. See original summary.

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented
Status: Active » Needs review
FileSize
entity-browser-behaviours-run-once-3055370-2.patch1.64 KB

Here is the first patch for this.

There is probably a similar problem with Drupal.behaviors.entityBrowserModal - that code will run on each behaviour trigger but I don't have time right now to check it.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

Thanks, I'll take a look as soon as possible.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
FileSize
expected entity browser display.png541.36 KB
actual entity browser display with the patch.png275.28 KB 
+++ b/js/entity_browser.modal.js
@@ -49,21 +49,41 @@
+        $document = $(context.contentDocument);

This definition is causing some trouble in one of our projects. Not sure why, but the $document is not set correctly and the call $document.on('dialogopen', '.ui-dialog',... does not get executed, hence the height of the entity browser does not get set. If I change $document to $(document) on that call it works fine.

See the expected vs actual images attached.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
Status: Needs review » Needs work
pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
entity-browser-behaviours-run-once-3055370-6.patch982 bytes

Took a new look into this after this bug and, hmmm, it seems i messed a stuff a bit. The idea is good, but execution not that good ;)

On top of this, it seems when the behaviour context is an iframe then `context.contentDocument.defaultView` will sometimes be set but sometimes it will be null so we can not use it in this case.

But after taking a second look on this, really there is no point using context to get a document and then use it. What we need is to always operate on the window which is a global object and its document, because here we need to do `once('fluid-modal')` check. The rest is not important.

Here is a new patch which should hopefully work now.

One more thing, i've noticed we could use https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimation... inside $window.resize() to optimize things even more, but that should be a follow-up.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
Status: Needs review » Reviewed & tested by the community

Agree that the context was a bit of a messup.

Tested locally and works fine, thanks for fixing @pivica.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented

Thanks, I'll review ASAP.

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
FileSize
multiple firings.png14.39 KB

Manually testing, it's easy to see it fires multiple times. On a clean install, if you enable the entity_browser_example module:

  Drupal.behaviors.fluidModal = {
    attach: function (context) {

      console.log('test');

It fires for each of the fields, so it shows in the console it fires seven times.

and the events fire six times when opening the dialog:

six times

oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
FileSize
with patch applied.png13.89 KB

With patch applied, the events fire only once each.

once each

  • oknate committed a10c241 on 8.x-2.x authored by pivica 
    Issue #3055370 by pivica, sasanikolic, oknate: Entity Browser fluidModal...

  • oknate committed 6a0fb5a on 8.x-1.x authored by pivica 
    Issue #3055370 by pivica, sasanikolic, oknate: Entity Browser fluidModal...
oknate’s picture

oknate
Location Greater New York City Area
CreditAttribution: oknate as a volunteer and commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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