Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Zooming Image Tool is broken #31436

Closed
2 tasks done
arbrandes opened this issue Dec 12, 2022 · 13 comments · Fixed by #36012
Closed
2 tasks done

The Zooming Image Tool is broken #31436

arbrandes opened this issue Dec 12, 2022 · 13 comments · Fixed by #36012
Assignees
Labels
bug Report of or fix for something that isn't working as intended

Comments

@arbrandes
Copy link
Contributor

arbrandes commented Dec 12, 2022

Description

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

Since then, attempts have been made to work around the problem. In the latest case, a PR fixes the loading of the tool, but it turns out that the script - which is hosted in a course at edx.org - is itself broken, as one can see in this comment.

It was originally proposed that this functionality be removed, to be later reimplemented as an XBlock. Doing so would make it possible to maintain and use the code properly, instead of relying on a remote third-party.

However, a whole XBlock just for a few lines of Javascript and a couple of images is likely too much, so an alternate plan was devised where all of the relevant pieces are inlined in the template itself. See this PR for the implementation.

PRs

@arbrandes arbrandes added this to the Palm.1 milestone Dec 12, 2022
@arbrandes arbrandes added the bug Report of or fix for something that isn't working as intended label Dec 12, 2022
@arbrandes arbrandes self-assigned this Dec 12, 2022
@jalondonot
Copy link

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

@arbrandes
Copy link
Contributor Author

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

@arbrandes arbrandes changed the title [HTML Block] The Zooming Image Tool template is broken [DEPR] HTML Block: The Zooming Image Tool template is broken May 30, 2023
@dianakhuang dianakhuang moved this from Proposed to Communicated in DEPR: Deprecation & Removal Jun 15, 2023
@dianakhuang dianakhuang moved this from Communicated to Proposed in DEPR: Deprecation & Removal Jun 15, 2023
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Aug 16, 2023

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
Copy link
Contributor

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

@cheskiduty
Copy link

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 arbrandes modified the milestones: Palm.1, Quince.1 Sep 5, 2023
@arbrandes
Copy link
Contributor Author

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

@arbrandes arbrandes moved this from Backlog to To Do in Axim Engineering Tasks Oct 6, 2023
@arbrandes arbrandes moved this from To Do to In Progress in Axim Engineering Tasks Oct 6, 2023
@arbrandes arbrandes moved this to In progress in Frontend Working Group Mar 6, 2024
@arbrandes arbrandes moved this from In Progress to To Do in Axim Engineering Tasks Mar 6, 2024
@arbrandes arbrandes modified the milestones: Quince.1, Redwood.1 Mar 19, 2024
@arbrandes arbrandes removed this from the Redwood.1 milestone May 9, 2024
@arbrandes arbrandes removed their assignment May 15, 2024
@arbrandes arbrandes moved this from In progress to Backlog in Frontend Working Group May 15, 2024
@feanil feanil self-assigned this Jul 3, 2024
@jignaciopm
Copy link
Contributor

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:

2024-10-15.22-48-55.mp4

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:

2024-10-15.23-05-57.mp4

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@arbrandes
Copy link
Contributor Author

As noted in the PR, while it fixes the loading of the script, the script itself - which is hosted by a course in edx.org - doesn't work correctly. Unless we want to include that script in the HTML block itself, I still think we should deprecate this template.

@pdpinch
Copy link
Contributor

pdpinch commented Dec 9, 2024

FYI, there's a discussion post with a workaround to the javascript failures: https://discuss.openedx.org/t/image-zoom-tool-workaround/9917

That said, I agree this template should be deprecated, unless and until it's moved to an xBlock with appropriate maintenance and support.

@arbrandes arbrandes changed the title [DEPR] HTML Block: The Zooming Image Tool template is broken [DEPR] HTML Block: Zooming Image Tool Dec 9, 2024
@arbrandes arbrandes changed the title [DEPR] HTML Block: Zooming Image Tool [DEPR] Zooming Image Tool (HTML block template) Dec 9, 2024
@arbrandes arbrandes moved this from Proposed to Removed in DEPR: Deprecation & Removal Dec 9, 2024
@arbrandes arbrandes moved this from In review to Done in Build-Test-Release Working Group Dec 9, 2024
@arbrandes arbrandes moved this from Backlog to Closed in Frontend Working Group Dec 9, 2024
@arbrandes arbrandes changed the title [DEPR] Zooming Image Tool (HTML block template) The Zooming Image Tool is broken Dec 11, 2024
@arbrandes arbrandes assigned arbrandes and unassigned feanil Dec 11, 2024
@arbrandes
Copy link
Contributor Author

Alright, here's something that seems to work!

#36012

@arbrandes arbrandes linked a pull request Dec 11, 2024 that will close this issue
@arbrandes arbrandes moved this from Closed to In progress in Frontend Working Group Dec 11, 2024
@arbrandes arbrandes moved this from Done to In progress in Build-Test-Release Working Group Dec 11, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Build-Test-Release Working Group Dec 13, 2024
@github-project-automation github-project-automation bot moved this from In progress to Closed in Frontend Working Group Dec 13, 2024
@arbrandes
Copy link
Contributor Author

Aaand the fixes have been merged, both to master and Sumac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

8 participants