Option for iFrame to automatically calculate height

I tested the patch #2 and it only works for me intermittently. I think when the iframe loads slowly it gets stuck on the spinner, despite the fact the iframe has finished loading and is hidden because it's height is stuck at 4

I guess set here, height must be being calculated at 0.

$(element).height(height + 4);

It needs to wait until the height is greater than zero. Perhaps a different event to react to? Perhaps a timeout? I'm not a front-end expert.

Also schema entity_browser.browser.display.iframe in config/schema/entity_browser.schema.yml will need updating, and ideally functional javascript test coverage, although that would tricky for sure.

Here's a screenshot of how it looks when it gets stuck.

- Fixes spinning issue in #4 by moving callback
- Fix states on config form, I think this changed when we dropped ctools?
- Add schema property and update tests to add default. I haven't added any test coverage (yet).

I just eyeballed the changes in the tests. I'll have to fix those later.

Updated the Modal display plugin. It extends the Iframe display plugin, so I had to unset the auto_height parameter in the default values and the configuration form.

Working on fixing the tests. Also I think the defaults could be simplified.

Adding some validation:

-    if ($form_state->getValue('height') <= 0) {
-      $form_state->setError($form['height'], $this->t('Height must be greater than 0.'));
+    if ($form_state->getValue('height') <= 0 && $form_state->getValue('auto_height') === 0) {
+      $form_state->setError($form['height'], $this->t('Height must be greater than 0, unless "Automatically Set Height" is checked.'));
     }

Trying to fix last test failure.

Trying to fix last failure. Sorry for spamming the issue. I can't get this test to work with chromedriver locally.

Related: https://stackoverflow.com/a/34432470/1214689

There must be a bug / logic error somewhere because the auto height is getting checked even though in the form it's set to '0', and I tried it with FALSE. When I test on my local environment, it's missing from the POST data when you submit the entity browser config and that checkbox is unchecked. I'm not sure what would be setting to TRUE. Investigating.

There's more info here:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

See #18 for follow up.

I noticed another issue. There's a white line (the iframe maybe?) over the throbber:

I don't think that's specific to this issue.

Follow up:

I added an issue for this: #3036813: Iframe partially eclipses throbber

OK, Following up #16. It seems that leaving the form element out fixes it. I was able to run the test locally with drupalci_testbot and removing the item from the drupalPostForm works (drupalPostForm is method to post drupal forms in functional tests), as it mimics the behavior of the browser not posting the form field for a checkbox that isn't checked.

For #17, I created a new issue: #3036813: Iframe partially eclipses throbber

Marking as needs work.

1) I think the resizing needs to keep reacting or to have some buffer. When using this within an entity embed dialog, I was getting a stuttering with mouseover on the submit button on the seven theme. This is due to the button adding shadow on hover which made the contents slightly larger, restoring the scrollbar, then when I moved the mouse off the button, the scrollbar disappeared. If I manually set the height of the iframe 10 pixels larger, this stuttering went away. So this requires a solution where it keeps calculating the height when the contents change. It's impossible to know what might change the height of the contents of the iframe and then restore the scrollbar.

2) If you close the entity browser and reopen it from field widget, it doesn't work the second time.

This issue is more complicated than I thought:

https://stackoverflow.com/questions/9975810/make-iframe-automatically-ad...

I think we could include the iframe resize library; https://stackoverflow.com/a/22991952/1214689

Changed javascript from event on adding of iframe to mutation observer. Now it will adjust height when the contents of the iframe change. See attached quicktime movie.

Manually tested, looks quite ok. Haven't noticed any obvious issues. Just a few nitpicks and questions.

1. +++ b/modules/example/config/install/entity_browser.browser.test_files.yml
   @@ -15,6 +15,8 @@ display_configuration:
      link_text: 'Select entities'
   

   Is this related?

2. +++ b/modules/example/config/install/entity_browser.browser.test_nodes.yml
   @@ -15,6 +15,8 @@ display_configuration:
      height: '500'
      link_text: 'Select entities'
   +  auto_open: false
   

   Same here.

3. +++ b/src/Plugin/EntityBrowser/Display/IFrame.php
   @@ -258,8 +271,23 @@ class IFrame extends DisplayBase implements DisplayRouterInterface {
   +        '#title' => $this->t('Automatically Set Height'),
   

   I think we don't use "word casing". So "Automatically set height.

Hi, thanks for the feedback, Primsi.

1) I add auto_open to the config for iframe in the test module, since the option is in the schema but missing in this config. It's not related and I could leave it out. But it seems good to add it.

diff --git a/modules/example/config/install/entity_browser.browser.test_files.yml b/modules/example/config/install/entity_browser.browser.test_files.yml
index c7a34ac..6294dd0 100644
--- a/modules/example/config/install/entity_browser.browser.test_files.yml
+++ b/modules/example/config/install/entity_browser.browser.test_files.yml
@@ -15,6 +15,8 @@ display_configuration:
   width: '650'
   height: '500'
   link_text: 'Select entities'
+  auto_open: false
+  auto_height: false

Here's the schema for iframe browser display:

entity_browser.browser.display.iframe:
  type: mapping
  label: 'iFrame display configuration'
  mapping:
    width:
      type: string
      label: 'iFrame width'
    height:
      type: string
      label: 'iFrame height'
    link_text:
      type: label
      label: 'Link text'
    auto_open:
      type: boolean
      label: 'Auto open'
    auto_height:
      type: boolean
      label: 'Automatically set iFrame height'

2) same thing here:

diff --git a/modules/example/config/install/entity_browser.browser.test_nodes.yml b/modules/example/config/install/entity_browser.browser.test_nodes.yml
index 55a0e2c..c788fea 100644
--- a/modules/example/config/install/entity_browser.browser.test_nodes.yml
+++ b/modules/example/config/install/entity_browser.browser.test_nodes.yml
@@ -15,6 +15,8 @@ display_configuration:
   width: '650'
   height: '500'
   link_text: 'Select entities'
+  auto_open: false
+  auto_height: false

3) Adding updated patch to fix capitalization.

Looks quite ok to me. Any other inputs?

The first review, tested in Chrome, for now, works fine. Here is some stuff i've found:

1. +++ b/js/entity_browser.iframe.js
   @@ -37,11 +37,12 @@
   +        'height': iframeSettings['auto_height'] ? 0 : iframeSettings['height'],
   

   Checked this a bit, seems to me that if we say for height value that it can also be 0 and then treat 0 value as the auto-height option we could then remove a lot of code around new auto_height value and still keep new auto height functionality.
   Tried this quickly locally and it seems it works fine.

   Any objections in doing this?

2. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,44 @@
   +        var iframeBody = $(iframe).contents().find('body')[0];
   

   We could maybe avoid using of jQuery here and write directly:

   iframe[0].contentDocument.body

3. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,44 @@
   +        var MutationObserver = window.MutationObserver || window.WebKitMutationObserver || window.MozMutationObserver;
   ...
   +        var iframeObserver = new MutationObserver (mutationHandler);
   

   Don't have a lot of exp with MutationObserver but seems to me that we can just directly write

   var iframeObserver = new MutationObserver (mutationHandler);

   and skip this line. More info on https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

4. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,44 @@
   +    $(element).height(0);
   

   Maybe avoid jQuery again and write

   element.style.height = '0px';
   

5. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,44 @@
   +    var document = element.contentDocument ? element.contentDocument : element.contentWindow.document,
   

   Again, checking the support for contentDocument seems to me we do not need to use contentWindow - i think it was used for IE8 or something like that?

6. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,44 @@
   +    // The 4 is an addition for MS browsers.
   

   Can we explain a bit better why we need this exactly in the comment?

   This is needed just for MS browser but will be applied for all browsers on the end?

Thanks for the feedback @pivica!

1) Very good idea to drop auto_height property all together. The modal settings form already has this behavior. This reduces the code changes enormously. I wish I had thought of this before! Excellent!

2) Sounds good, changed

-        var iframeBody = $(iframe).contents().find('body')[0];
+        var iframeBody = iframe[0].contentDocument.body;

3) I don't want to mess with the mutation observer. I don't fully understand it either. Leaving alone for now.

4) Sounds good, changed

-    $(element).height(0);
+    element.style.height = '0px';

5) Sounds good, changed

-    var document = element.contentDocument ? element.contentDocument : element.contentWindow.document,
+    var document = element.contentDocument,

6) Alas, I didn't write this code, it comes from #2, jzavrl. So leaving for now. I don't know what the extra 4 pixels are for. I don't think it hurts anything for now.

Great stuff @oknate, patch went from 19kb to 3kb :)

1. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,43 @@
   +    console.log(drupalSettings.entity_browser.iframe);
   +    if (drupalSettings.entity_browser.iframe[uuid].height == 0) {
   

   Debug statement left.

2. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,43 @@
   +        var MutationObserver = window.MutationObserver || window.WebKitMutationObserver || window.MozMutationObserver;
   ...
   +        var iframeObserver = new MutationObserver (mutationHandler);
   

   The WebKitMutationObserver and MozMutationObserver are here just to support Firefox <14, Chrome <27 and Safari <6.1. We are quite far from this so we can just write:

   var iframeObserver = new MutationObserver(mutationHandler);

   and remove the previous line.

3. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,43 @@
   +    $(element).height(height + 4);
   

   I'll test this a bit then with IE11 and try to figure why we need this.

4. +++ b/src/Plugin/EntityBrowser/Display/IFrame.php
   @@ -256,8 +256,8 @@ class IFrame extends DisplayBase implements DisplayRouterInterface {
   -      '#min' => 1,
   

   What will happen if user write negative value here?

Thanks for the feedback, @pivica.

1) removed debug statement
2) removed line "var MutationObserver = window.MutationObserver || window.WebKitMutationObserver || window.MozMutationObserver;"
3) It would be nice if we can figure out what the 4px are about, thanks
4) I have added a #min of zero on the height form element. I also updated the modal, since that has the same issue. It allows negative numbers.
5) I also removed the variable iFrameBody, since it's only used once:

-        var iframeBody = iframe[0].contentDocument.body;
+        iframeObserver.observe (iframe[0].contentDocument.body, obsConfig);

1. +++ b/src/Plugin/EntityBrowser/Display/IFrame.php
   @@ -256,8 +256,9 @@ class IFrame extends DisplayBase implements DisplayRouterInterface {
   +      '#description' => $this->t('Empty value for responsive height.'),
   

   Not sure should we improve description here to note that height will adapt to the content height of the iframe, because this could mean that it is adapting to browser height?

2. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,39 @@
   +        var iframeObserver = new MutationObserver (mutationHandler);
   +        iframeObserver.observe (iframe[0].contentDocument.body, obsConfig);
   

   JS coding standard says no space for function calls https://www.drupal.org/docs/develop/standards/javascript/javascript-codi....

3. +++ b/js/entity_browser.iframe.js
   @@ -53,8 +54,39 @@
   +    // The 4 is an addition for MS browsers.
   +    $(element).height(height + 4);
   

   Did IE11 test and i don't see any special change adding this 4px. Unless we get some hint why this is done I would suggest we remove it for now, we can always add it later in follow-up. Here are the screen-shots:

   With 4px:
   

   Without 4px:
   

1) Updated description. That's a good point.

-      '#description' => $this->t('Empty value for responsive height.'),
+      '#description' => $this->t('Leave blank for iFrame to dynamically adjust height to contents.'),

2) removed space

-        iframeObserver.observe (iframe[0].contentDocument.body, obsConfig);
+        iframeObserver.observe(iframe[0].contentDocument.body, obsConfig);

3) removed 4px and comment. Thanks for the info and screenshot.

-
-    // The 4 is an addition for MS browsers.
-    $(element).height(height + 4);
+    $(element).height(height);

+++ b/js/entity_browser.iframe.js
@@ -41,7 +41,8 @@
+        'class': 'entity_browser_iframe'

Sorry i forgot to ask about this one, why we are adding this class here when there is no code (JS/CSS) that is using it?

Not sure should we remove this, but if we want to add it for the case of some styling for the future at least we should use proper class name which would be entity-browser-iframe.

Good point. Removing the unused class.

-        'id': 'entity_browser_iframe_' + iframeSettings['entity_browser_id'],
-        'class': 'entity_browser_iframe'
+        'id': 'entity_browser_iframe_' + iframeSettings['entity_browser_id']

This was left over from original patch #2, but isn't used in the current patch.

From the #2:

+      $(context).find('iframe.entity_browser_iframe').once('iframe-auto-height').on('load', function () {
+        var uuid = $(this).attr('data-uuid');

> This was left over from original patch #2, but isn't used in the current patch.

Thx for explaining, i guess it does not hurt leaving CSS class for any possible styling need. We do the same for modal version so its fine we do the same for iframe version.

Found one additional small issue, after that, i think we are finally good to go :)

+++ b/js/entity_browser.iframe.js
@@ -53,8 +53,37 @@
+    element.style.height = '0px';
...
+    $(element).height(height);

We should use the same no jQuery syntax here. Actually, the first statement does not do anything now because we always set calculated height so we can remove line 82 and just keep

element.style.height = height + 'px';

Here is the updated patch and interdiff.txt as per comment #37

I'm pretty sure we need line 82. It's needed to reset the iframe before calculating the height of the contents, otherwise the callback won't work any more, because it picks up the height of the iframe in the calculation.

One of these three takes the height of the iframe into account:
html.clientHeight, html.scrollHeight, html.offsetHeight

I think we could maybe remove those from the calculation:

    var document = element.contentDocument,
      body = document.body,
      height = Math.max(body.scrollHeight, body.offsetHeight);

Then we could drop the initial reset to zero of the iframe. I'm reluctant to mess with it, since I don't fully understand why those were originally included in the calculation.

I found a bug with this in the entity embed dialog. It seems the order of operations was a problem and the modal was already positioned before the iframe contents loads.

An easy fix was to add a $(window).trigger('resize'); after resizing. I think we could add conditional code that detects if the iframe is within a modal before doing this. I'm not sure of the best way to do that.

I found a better way to resize the modal. entity_embed.dialog.js has this code:

Drupal.behaviors.entityEmbedDialog = {
    attach: function (context, settings) {
      $('body').once('js-entity-embed-dialog').on('entityBrowserIFrameAppend', function () {
        $('.entity-select-dialog').trigger('resize');
        // Hide the next button, the click is triggered by Drupal.entityEmbedDialog.selectionCompleted.
        $('#drupal-modal').parent().find('.js-button-next').addClass('visually-hidden');
      });
    }
  };

So I borrowed $('.entity-select-dialog').trigger('resize'); and it seems to work well.

See 46 for interdiff. Added later by request.

> Then we could drop the initial reset to zero of the iframe. I'm reluctant to mess with it since I don't fully understand why those were originally included in the calculation.

Yeah, that is the problem when the code is added without any comment explaining why it is there ;)
I guess we will need to test this a bit more and try to figure out why/how the stuff works, change it if needed and add comments.

> So I borrowed $('.entity-select-dialog').trigger('resize'); and it seems to work well.

Bit confused now because i thought iframe JS code is loaded only when we show inline iframe entity browser and it is not related to the modal dialog?

Will check this later during the day.

@oknate can i ask you to start providing interdiff when you upload patches. I know it's a bit of a hassle and requires a bit more time but reviewing the changes is then much easier.

> An easy fix was to add a $(window).trigger('resize'); after resizing.

Ah sorry, coffee still didn't start to work :) We are triggering window resizing so that is not related to dialog thing. So ignore my previous question related to this.

Per your request, here's the change from 40 to 41.

The modal uses an iframe. The entity embed module you can specify, if I remember correctly, if you don't use iframe, you get a second modal. But using an iFrame in entity embed is definitely an option, and its the option I have been using for entity embed buttons.

I think this is ready to go in. Can we get some people to vouch for this as RTBC?

I tested the patch #41 and it only works for me. Screenshot attached for the working copy.

Thanks for the Patch @oknate.

I faced 2 issues
1. Loader hanged after selecting image.
2. Iframe height is getting larger than required for the collapsed view.

Can you give more information on how to recreate the bugs mentioned above, @vijay.cgs?

Does removing the patch fix the hanging issue?

What field type is this that has a "place" button. What plugins are you using to the entity browser?

I was seeing the stuttering again (see #19), when in the context of an entity embed and the hovering over a button added shadow to the button.

Now that I think about it, we don't want the scrollbars to appear when using this option. So this update sets scrolling="no" on the iframe when this option is enabled.

Patch for #52

From #31

> 3) It would be nice if we can figure out what the 4px are about, thanks

It seems this came from the fact that by default IFrame is using display inline rule. In #3055358: Entity Browser iFrame 100% height can cause vertical scrollbar there is a link to https://stackoverflow.com/a/9131632 and there it is explained in more details:

To summarize it:

- white space before causes 4px white space at the rigth of the iframe.
- white space after csuses 4px white space after the iframe.

This is due to the inline character of iframe as pointed out in the first post.

So it was a good decision to remove 4px.

I've had the same issue as @vijay.cgs where the UI hangs.
This only happens in my case when the browser is not set to be open by default in the media field widget settings.

https://www.dropbox.com/s/awbmmjtas98aq4y/Screenshot%202019-06-20%2012.3...

I've updated the patch to avoid resizing things to zero which would happen when opening the wrapping details element making it seem like everything has hung.

@thtas, that is great to hear! Is there any way you can add an interdiff text file, or at least put in comment #57 the changes you made?

Hi,
I tested patch #57 and it works fine. But there is a problem with reducing the height.
I use a table view with 10 entries, when paging to the last page it may as < 10 entries and the iframe should resize but it doesn't.

I followed comment #39 and removed html.clientHeight, html.scrollHeight, html.offsetHeight from the calculation and now it works. But otherwise great work!

and I added

--- entity_browser/js/entity_browser.iframe.js
+++ entity_browser/js/entity_browser.iframe.js
@@ -67,6 +67,7 @@
         function mutationHandler () {
           Drupal.entityBrowserIFrame.setIframeHeight(iframe[0]);
         }
+        window.onresize = mutationHandler;
       });
     }

to call the iframe to resize when the browser is resized or mobiles change their orientation

@oknate there is an interdif, not sure why you're not seeing it: https://www.drupal.org/files/issues/2019-06-20/interdiff.txt

@mvogel yeah i'm happy with the #57 patch for my needs but i'm not sure it's the best solution. I'm sure there is a reason for the 0px height I'm not aware of...

diff --git a/entity-browser-auto-height-iframe-3025100-52.patch b/entity-browser-auto-height-iframe-3025100-52.patch
new file mode 100644
index 0000000..ccb385e
--- /dev/null
+++ b/entity-browser-auto-height-iframe-3025100-52.patch

#57 went to ~9KB compared to #53. We do not need the previous .patch file in the current patch.

Fixes the issue mentioned in #61

Also ran into this, here's my solution, a bit more simple. May not apply as it's used after 4 other patches but easy to re-roll.

Sorry, wrong file.

Patch #64 unfortunately do not apply anymore to the last dev version (26.06.2022)

Rerolled.

This approach (calling the resize script from an onload event on the iframe) works more consistently for me across different browsers.

Getting Console error after applying #67 patch, Null value in "iframe.contentWindow".
We need to add a condition for this.

// Resize iFrame based on content (called from iframe onload).
function resizeIframe(iframe) {
// Remove default height so we're not calculating from that.
iframe.style.height = '';
// Set height to content.
iframe.style.height = iframe.contentWindow.document.documentElement.scrollHeight + 'px';
// Periodically auto-resize as contents may change.
setInterval( function () {
if(iframe.contentWindow == null) return;
iframe.style.height = iframe.contentWindow.document.documentElement.scrollHeight + 'px';
}, 1000);

I've not run in to the issue from #68, but I do have a problem when uploading a large image when the browser window is not very high, resulting the modal being resized and the bottom of it gets hidden below the fold. Making the button at the submit button un-clickable. To fix it you have to re-size the window (by just a tiny amount) and the modal position is re-calculated correctly.

I've added a resize event trigger to the resizeIframe method which fixes it for my use-case, but this whole thing is pretty stinky. use at your own risk.

added small validation to prevent errors