Mod_Wrapper multiple instances

[brianteeman]

Pull Request for Issue #17425

This is a PARTIAL fix. It addresses the issue of the unique id for each occurrence of a wrapper module but as noted in the original issue the JS is broken as well

It looks to me that we can change the selector to the following

var iframe = document.querySelectorAll('*[id^="blockrandom"]');

but there are still issues and I am NOT a js guru (pinging @dgt41 )

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

[Quy]

I have tested this item ✅ successfully on 0f908f0

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19136.}

[dgrammatiko]

Just turn that Id to a class to solve the multiple instances problem (it’s a b/c break by the way).
About the chrome error: it’s expected unless you properly set the allowed scripts in your site. There is nothing to fix here

[brianteeman]

Sorry I don't understand you?

[dgrammatiko]

Either your approach here or a class driven solution is b/c break! Think that someone got an override then either solution will break the overrides...

[brianteeman]

turning the id to a class would be an a11y fail as every iframe has to have a unique id which was the whole point of the pr

[dgrammatiko]

@brianteeman can you please add also the js part (the code in the desc is fine)

[brianteeman]

js added

[C-Lodder]

You can still have an ID on the iframe. Just add a class too and target the class in the JS as opposed to the ID

[Sophist-UK]

After applying this fix I get:

iframe-height.js:5 Uncaught TypeError: Cannot use 'in' operator to search for 'contentDocument' in null
    at iFrameHeight (iframe-height.js:5)
    at HTMLIFrameElement.onload ((index):809)

Page has two iframes - one within com_wrapper, and one inserted into a module by a different plugin.

See #19337 for environment.

[brianteeman]

iirc the code i wrote here only addresses multiple instances of the wrapper module. your bug sounds unrelated

[Quy]

Remove spaces at the end of the line.

[Sophist-UK]

IMO the call to iframeHeight should be made with the ID as a parameter so that the JS can find the absolutely correct iframe.

[Sophist-UK]

iirc the code i wrote here only addresses multiple instances of the wrapper module. your bug sounds unrelated

No - the error message I reported after trying this fix only appears with this fix and is unrelated to the cross-domain issue I have reported separately.

[Fedik]

iirc the code i wrote here only addresses multiple instances of the wrapper module. your bug sounds unrelated

it is relevant 😉
after your changes var iframe contain multiple iframe element instead of 1, but whole code expect 1 element.
The code need more changes to support multiple instances.

[Fedik]

@brianteeman I am not tested, but this should work: ignore it

function iFrameHeight()
{
    var iframe, doc, 
        height  = 0,
        isAll   = !!document.all,
        iframes = document.querySelectorAll('iframe[id^="blockrandom"]');

    for (var i = 0, l = iframes.length; i < l; i++) {
        iframe = iframes[i];
        doc    = 'contentDocument' in iframe ? iframe.contentDocument : iframe.contentWindow.document;
        height = parseInt(doc.body.scrollHeight);

        if (!isAll)
        {
            iframe.style.height = height + 60 + 'px';
        }
        else
        {
            document.all[iframe.id].style.height = height + 20 + 'px';
        }
    }
}

[Sophist-UK]

The problem with this code is that it will run on every iframe every time any iframe finishes loading.

As stated previously IMO the code should call iFrameHeight with the id as a parameter iframeHeight(id) and the code should then read:

function iFrameHeight(id)
{
    var height = 0;
    var iframe = document.getElementById('blockrandom-' + id);
    var doc    = 'contentDocument' in iframe ? iframe.contentDocument : iframe.contentWindow.document;

    if (!document.all)
    {
        height = doc.body.scrollHeight;
        iframe.style.height = parseInt(height) + 60 + 'px';
    }
    else if (document.all)
    {
        height = doc.body.scrollHeight;
        document.all.blockrandom.style.height = parseInt(height) + 20 + 'px';
    }
}

[Fedik]

okay, I see,
then it can be even easier

[Fedik]

@brianteeman check brianteeman#67

[brianteeman]

@Sophist-UK you asked for changes, changes have been made so please test

[Sophist-UK]

I have tested this - still getting the x-domain console error which is to be expected, but aside from that it seems to work.

[Quy]

@Sophist-UK Please mark your test result here: https://issues.joomla.org/tracker/joomla-cms/19136

[Sophist-UK]

I have tested this item ✅ successfully on d81bea8.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19136.}

[Quy]

I have tested this item ✅ successfully on d81bea8

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19136.}

[Quy]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19136.}

[brianteeman]

Thanks