openedx / edx-platform

The Open edX LMS & Studio, powering education sites around the world!
https://openedx.org
GNU Affero General Public License v3.0
7.43k stars 3.88k forks source link

[DEPR] HTML Block: The Zooming Image Tool template is broken #31436

Open arbrandes opened 1 year ago

arbrandes commented 1 year ago

Description

During release testing for Olive, it was discovered by the Build Test Release group that the Zooming Image Tool is currently broken.

The feature works as expected in the Studio preview, but clicking on the image in the LMS simply opens the image in another browser tab.

To reproduce

  1. Fire up a devstack, open the demo course in Studio and add a Text block to any unit.
  2. Select the Zooming Image Tool template and publish the unit; there's no need to edit the contents.
  3. Test the resulting tool in Studio itself: the zoom should work as expected.
  4. Open the unit in the LMS. Clicking the image will open it in a new tab, as opposed to in the zoomed tab, and the console will display the following error:
Uncaught ReferenceError: JavascriptLoader is not defined

Attempts to work around the issue

An attempt was made to work around the problem by changing the original <div ... data-src=loupe.js /> to <script ... src=loupe.js />. It gets rid of the console error, but the wrong behavior remains.

Proposal

It is proposed that the tool be removed, since it is not only broken with no clear fix but it introduces a potential security issue by officially recommending bringing in external javascript.

PRs

jalondonot commented 1 year ago

Hi @arbrandes. Have you had the chance to keep working on this issue? how could we help to unblock it?

arbrandes commented 1 year ago

I have not, unfortunately. I'll see about taking this to the DEPR working group.

mariajgrimaldi commented 1 year ago

This is marked for Palm.1, released a couple of months ago. Is it realistic to change it to the Palm.3 milestone or Quince?

dianakhuang commented 1 year ago

@arbrandes Could you announce this deprecation so that we can move it to 'Communicated'?

cheskiduty commented 1 year ago

The Zooming Image Tool template is broken because it uses a deprecated feature of HTML called "HTML blocks". What are HTML blocks, why were they deprecated, and how can we fix the template to avoid using them?

HTML blocks were introduced in HTML5 as a way to allow authors to create self-contained blocks of content that could be easily styled and laid out on a web page. However, they were never widely adopted and have since been deprecated in favor of other layout methods such as CSS Flexbox and Grid.

One reason why HTML blocks were deprecated is that they can cause compatibility issues with older browsers and assistive technologies. For example, some screen readers may not be able to properly read the contents of an HTML block, making it difficult or impossible for users with disabilities to access the content.

To fix the Zooming Image Tool template and avoid using HTML blocks, we can replace them with CSS Flexbox or Grid. For example, we can wrap the image and its associated text in a container element and use CSS Flexbox to align the elements horizontally and vertically. Here's an example of how the updated code might look:

arbrandes commented 1 year ago

@cheskiduty, if you're up to creating a PR to demonstrate the solution (say, in the next couple of weeks), that would be great! Otherwise, I'm afraid we'll go ahead with the deprecation.

@dianakhuang, @mariajgrimaldi, I think we should target Quince, at this point.

jignaciopm commented 3 weeks ago

Hi @arbrandes I have been following up on this issue and am in charge of it from Edunext.

The first thing I have done is test the proposed workaround. I made small modifications to the code, it left a function like this:

const zoomImgLoupe = ({
    mainClassName = 'zoom_image',
    imageZoomFrameClassName = 'image_zoom_frame',
    loupeWidth = 200,
    loupeHeight = 200,
} = {}) => {
    const updateFuncs = [];
    let mouseE;
    function addUpdateFunc(func) {
        updateFuncs.push(func);
    }
    function removeUpdateFunc(func) {
        updateFuncs.splice(updateFuncs.indexOf(func), 1);
    }
    function callUpdateFuncs() {
        for (const func of updateFuncs) {
            func();
        }
    }
    function updateMouseE(e) {
        mouseE = e;
    }
    function update() {
        callUpdateFuncs();
        reqUpdate();
    }
    function reqUpdate() {
        window.requestAnimationFrame(update);
    }
    function getTotalPos(oEl) {
        const oRect = document.body.getBoundingClientRect();
        const tRect = oEl.getBoundingClientRect();
        return [tRect.left - oRect.left, tRect.top - oRect.top];
    }
    function findClass(ele, targetClass) {
        if (typeof ele.className === 'string') {
            return ele.className.includes(targetClass);
        }
    }
    const zoomImgs = [];
    function getZoomData(img) {
        let res;
        for (let i = 0; i < zoomImgs.length; i++) {
            const data = zoomImgs[i];
            if (data.img === img) {
                res = data;
                break;
            }
        }
        return res;
    }
    function addZoomImg(img) {
        if (getZoomData(img) !== undefined) {
            console.error('!!! TRIED TO ADD ALREADY EXISTING IMG !!!');
            return;
        }

        const data = {
            img: img,
        }

        function upd() {
            const zoomScale = Number(data.img.dataset.zoom_scale);
            const pointerOnWindowX = mouseE.clientX + window.scrollX;
            const pointerOnWindowY = mouseE.clientY + window.scrollY;
            const [imageOnWindowX, imageOnWindowY] = getTotalPos(data.img);
            const loupeOnPointerY = window.innerHeight - mouseE.clientY < loupeHeight ? -loupeHeight-10 : 0;
            data.zoomImg.style.width = (data.img.offsetWidth * zoomScale).toString() + 'px';
            data.zoomImg.style.height = (data.img.offsetHeight * zoomScale).toString() + 'px';
            data.hoverDiv.style.transform = `translate(-50%, ${loupeOnPointerY}px) translate(${pointerOnWindowX}px, ${pointerOnWindowY}px)`;
            data.zoomImg.style.transform = `translate(${(data.hoverDiv.offsetWidth * 0.5).toString()}px, ${(data.hoverDiv.offsetHeight * 0.5).toString()}px) translate(${(imageOnWindowX - pointerOnWindowX) * zoomScale}px, ${(imageOnWindowY - pointerOnWindowY) * zoomScale}px)`;
        }

        img.style.cursor = 'crosshair';

        img.onmouseenter = () => {
            const hoverDiv = document.createElement('div');
            hoverDiv.className = imageZoomFrameClassName;
            hoverDiv.style.cssText = `all: initial; width: ${loupeWidth}px; height: ${loupeHeight}px; position: absolute; border-radius: 50%; box-shadow: 0 0 10px 0 rgba(0, 0, 0, 0.5); pointer-events: none; overflow: hidden; z-index: 999; background-color: white;`;
            document.body.insertBefore(hoverDiv, document.body.firstChild);
            data.hoverDiv = hoverDiv;

            const zoomImg = document.createElement('img');
            zoomImg.className = 'mag_image';
            zoomImg.src = img.dataset.zoom_image || img.src;
            zoomImg.style.cssText = 'all: initial; pointer-events: none;';
            zoomImg.style.pointerEvents = 'none';

            hoverDiv.appendChild(zoomImg);
            data.zoomImg = zoomImg;
            addUpdateFunc(upd);
        }

        img.onmouseleave = () => {
            removeUpdateFunc(upd);
            data.hoverDiv.remove();
        }
    }
    function childAdded(child) {
        if (findClass(child, mainClassName)) {
            addZoomImg(child)
        }
    }
    const observer = new MutationObserver((mutationList) => {
        for (const mutation of mutationList) {
            if (mutation.type === 'childList') {
                for (const node of mutation.addedNodes) {
                    if (node.nodeType === 1) {
                        childAdded(node);
                    }
                }
            }
        }
    });
    function newChild(cChild) {
        childAdded(cChild);
        searchChildren(cChild);
    }
    function searchChildren(oChild) {
        const children = oChild.children;
        for (let i = 0; i < children.length; i++) {
            const cChild = children[i];
            newChild(cChild);
        }
    }
    const observerEle = document.body;
    newChild(observerEle);
    observer.observe(observerEle, {childList: true, subtree: true});
    document.onmousemove = updateMouseE;
    reqUpdate();
}

and use the following HTML

<img
    class="zoom_image"
    src="https://studio.edx.org/c4x/edX/DemoX/asset/pathways_detail_01.png"
    data-zoom_scale="2.5"
/>

The result of implementing this in the LMS was the following:

https://github.com/user-attachments/assets/ca177f1a-859b-4e43-919f-5dd7153b5cc2

[!NOTE]
However, when trying to implement this, I ran into the same problem

Uncaught ReferenceError: JavascriptLoader is not defined

Therefore, I do not think this workaround is a suitable solution.

In the process I was working on understanding the root problem, and it really is something simple, we are trying to call a function which has not yet been loaded in the DOM, therefore, I had to find a way to replace the form in which the jquery.loupeAndLightbox.js utility was loaded

<div class="script_placeholder" data-src="https://studio.edx.org/c4x/edX/DemoX/asset/jquery.loupeAndLightbox.js"></div>

Coming to the following alternative:

<script type="text/javascript">// <![CDATA[
    function loadScript(url, callback) {
        let script = document.createElement("script");
        script.type = "text/javascript";

        if (script.readyState) {  // IE
            script.onreadystatechange = function() {
                if (script.readyState === "loaded" || script.readyState === "complete") {
                    script.onreadystatechange = null;
                    callback();
                }
            };
        } else {  // Other browsers
            script.onload = function() {
                callback();
            };
        }

        script.src = url;
        document.head.appendChild(script);
    }

    function callback() {
        window.onload = zoomImgLoupe()
    }

    loadScript("/assets/zoomImgLoupe.js",callback);
// ]]></script>

This code first load the script through the URL sent in loadScript and then execute the callback. In this way, we ensure that the function definition has been loaded before the call or reference to it.

With this alternative, I have made the original utility work in LMS jquery.loupeAndLightbox.js:

https://github.com/user-attachments/assets/13b0c00e-e139-438b-8df1-e1d480755d84

I think this would be the correct workaround, because we don't have a real problem in the jquery.loupeAndLightbox.js library, but rather in how the script that defines the function is being loaded.

arbrandes commented 3 weeks ago

@jignaciopm, thanks a lot for the investigation and solution! It certainly looks good to me! Would you be able to submit a PR to fix the issue?

jignaciopm commented 3 weeks ago

Yes @arbrandes of course. It is here. I await your review.