? ? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
22 Dec 2017

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

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
1.00

0f908f0 22 Dec 2017 avatar brianteeman cs
avatar brianteeman brianteeman - open - 22 Dec 2017
avatar brianteeman brianteeman - change - 22 Dec 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Dec 2017
Category Modules Front End
avatar Quy Quy - test_item - 22 Dec 2017 - Tested successfully
avatar Quy
Quy - comment - 22 Dec 2017

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.

avatar dgt41
dgt41 - comment - 22 Dec 2017

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

avatar brianteeman
brianteeman - comment - 22 Dec 2017

Sorry I don't understand you?

avatar dgt41
dgt41 - comment - 22 Dec 2017

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...

avatar brianteeman
brianteeman - comment - 22 Dec 2017

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

avatar dgt41
dgt41 - comment - 23 Dec 2017

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

avatar brianteeman brianteeman - change - 23 Dec 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 23 Dec 2017
Category Modules Front End JavaScript Modules Front End
avatar brianteeman
brianteeman - comment - 23 Dec 2017

js added

avatar C-Lodder
C-Lodder - comment - 23 Dec 2017

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

avatar Sophist-UK
Sophist-UK - comment - 8 Jan 2018

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.

avatar brianteeman
brianteeman - comment - 9 Jan 2018

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

avatar Sophist-UK
Sophist-UK - comment - 9 Jan 2018

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

avatar Sophist-UK
Sophist-UK - comment - 9 Jan 2018

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.

avatar Fedik
Fedik - comment - 9 Jan 2018

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.

avatar Fedik
Fedik - comment - 9 Jan 2018

@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';
        }
    }
}
avatar Sophist-UK
Sophist-UK - comment - 9 Jan 2018

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';
    }
}
avatar Fedik
Fedik - comment - 9 Jan 2018

okay, I see,
then it can be even easier

avatar Fedik
Fedik - comment - 9 Jan 2018
avatar brianteeman
brianteeman - comment - 16 Jan 2018

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

avatar Sophist-UK
Sophist-UK - comment - 16 Jan 2018

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

avatar Quy
Quy - comment - 16 Jan 2018
avatar Sophist-UK
Sophist-UK - comment - 16 Jan 2018

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.

avatar Quy Quy - test_item - 16 Jan 2018 - Tested successfully
avatar Quy
Quy - comment - 16 Jan 2018

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.

avatar Quy Quy - alter_testresult - 16 Jan 2018 - Sophist-UK: Tested successfully
avatar Quy Quy - alter_testresult - 16 Jan 2018 - Sophist-UK: Tested successfully
avatar Quy Quy - change - 16 Jan 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 16 Jan 2018

RTC


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

avatar mbabker mbabker - change - 16 Jan 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-01-16 22:40:06
Closed_By mbabker
Labels Added: ?
avatar mbabker mbabker - close - 16 Jan 2018
avatar mbabker mbabker - merge - 16 Jan 2018
avatar brianteeman
brianteeman - comment - 16 Jan 2018

Thanks

Add a Comment

Login with GitHub to post a comment