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

dome to image not working in iphone #192

Open
atiqi36 opened this issue Oct 8, 2024 · 12 comments
Open

dome to image not working in iphone #192

atiqi36 opened this issue Oct 8, 2024 · 12 comments

Comments

@atiqi36
Copy link

atiqi36 commented Oct 8, 2024

Hi,

I am using latest version of domtoimage and it works fine everywhere except iPhone. You can check it here on the live site https://www.thecanvasprints.co.uk/canvas?action=upload if you upload the image and click Add to Cart, it should open the preview in a popup. The preview open code is this;

var node = document.getElementById("canvasImageWrapper");
domtoimage.toPng(node, {
  width: node.offsetWidth,
  height: node.offsetHeight
})
  .then((dataurl) => {
    $("#previewModal .modal-title").text("Preview")
    $("#previewModal .alert-info").show()
    $("#custom-size-form").hide()
    $(".modal-content1 #previewImage").show()
    $("#preview-btn-cont.modal-footer").show()
    $("#custom-btn-cont.modal-footer").hide()

    return dataurl;
  })

The preview works fine in all browsers except if you check it on actual iPhone. It works fine if you view in chrome in iPhone mode but don't work on actual iPhone.

Please help.

@ecancil
Copy link

ecancil commented Oct 13, 2024

I think i finally got to the bottom of this.

Its the way theyre using inside svg, loading it to an image, and then writing the image to a canvas, and grabbing the objectURL from the canvas.

In webkit, particularly mobile safari (i think youll see similar in PC safari as well) it does not like images with svgs loaded into them that are then written to a canvas.

I have updated this method in the library

function makeImage(uri) {
            if (uri === 'data:,') {
                return Promise.resolve();
            }

            return new Promise((resolve, reject) => {
                // Create an SVG element to house the image
                const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
                const svgImage = document.createElementNS('http://www.w3.org/2000/svg', 'image');

                // Set attributes for the SVG image
                svgImage.setAttribute('href', uri);

                if (domtoimage.impl.options.useCredentials) {
                    svgImage.setAttribute('crossOrigin', 'use-credentials');
                }

                // Append the image to the SVG
                svg.appendChild(svgImage);

                // Add the SVG to the document body (invisible)
                document.body.appendChild(svg);

                svgImage.onload = function () {
                    // Cleanup: remove the SVG from the document
                    document.body.removeChild(svg);

                    // Resolve the promise with the SVG image
                    if (window && window.requestAnimationFrame) {
                        window.requestAnimationFrame(() => {
                            resolve(svgImage);
                        });
                    } else {
                        resolve(svgImage);
                    }
                };

                svgImage.onerror = (error) => {
                    // Cleanup: remove the SVG from the document
                    document.body.removeChild(svg);

                    // Reject the promise with the error
                    reject(error);
                };
            });
        }
        ```
        
        and I've largely had success (i still snap two times in a row just in case - but this seems to -always- fix it)

@IDisposable
Copy link
Member

IDisposable commented Oct 14, 2024

Interesting change... are we sure this works for all incoming uri possibilities and doesn't introduce other issues?

I might a beta release for you to test, but I would really like a simple repro JSFiddle to ensure I have something to test against you use case.

I also want to adjust to cloning/add order a bit as (as far as my testing shows), we should set the attributes and callbacks before adding the image to the SVG wrapper, and the SVG wrapper to the body (experience shows adding thing first seldom triggers the callbacks or applies the properties as expected).

I'm also wondering if we should put this new behaviour behind an option flag (or perhaps at minimum keeping the old behaviour as an opt-in fallback option)

@IDisposable
Copy link
Member

... just FYI, you can easily create your own makeImage as shown above, the force the (unchanged) dom-to-image-more to use it by overriding the domtoimage.impl.util.makeImage value to point to your version.

@IDisposable
Copy link
Member

IDisposable commented Oct 14, 2024

Is the double SVG really needed? Can we just create the image your way (e.g. document.createElementNS('http://www.w3.org/2000/svg', 'image') instead of new Image()) add/remove it from the document.body as needed?

Also, coded this way, the image is now part of the <body> so all the CSS will leak through... going to really have to test this to see what it breaks (or simplifies)

@IDisposable
Copy link
Member

IDisposable commented Oct 14, 2024

I'm suggesting perhaps this might work just as well for your use case... no extra SVG wrapper (just creating a SVG element instead of an Image) ... and adding/removing to the body...

EDIT: NOPE. This only rendered a blank SVG... no support for non-SVG. More to come.

        function makeImage(uri) {
            if (uri === 'data:,') {
                return Promise.resolve();
            }

            return new Promise(function (resolve, reject) {
                // Create an SVG element to house the image
                const image =  document.createElementNS('http://www.w3.org/2000/svg', 'image'); /* was new Image(); */

                if (domtoimage.impl.options.useCredentials) {
                    image.crossOrigin = 'use-credentials';
                }

                image.onload = function () {
                    if (window && window.requestAnimationFrame) {
                        // In order to work around a Firefox bug (webcompat/web-bugs#119834) we
                        // need to wait one extra frame before it's safe to read the image data.
                        window.requestAnimationFrame(function () {
                            resolve(image);
                        });
                    } else {
                        // If we don't have a window or requestAnimationFrame function proceed immediately.
                        resolve(image);
                    }

                    // Cleanup: remove the SVG from the document
                    document.body.removeChild(image);
                };
                
                image.onerror = (error) => {
                    reject(error);

                    // Cleanup: remove the SVG from the document
                    document.body.removeChild(image);
                };

                // Add the SVG to the document body (invisible)
                document.body.appendChild(image);
                image.src = uri;
            });
        }

@IDisposable
Copy link
Member

image

@IDisposable
Copy link
Member

IDisposable commented Oct 14, 2024

So, this line breaks (almost) all unit-tests... const svgImage = document.createElementNS('http://www.w3.org/2000/svg', 'image');

Swapping back to const image = new Image(); and wrapping in an outer SVG as patterned by your suggestion (and reordered) all passes.. does THIS help YOUR use case?

        function makeImage(uri) {
            if (uri === 'data:,') {
                return Promise.resolve();
            }

            return new Promise(function (resolve, reject) {
                // Create an SVG element to house the image
                const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
                
                // and create the Image element to insert into that wrapper
                const image = new Image();

                if (domtoimage.impl.options.useCredentials) {
                    image.crossOrigin = 'use-credentials';
                }

                image.onload = function () {
                    // Cleanup: remove theimage from the document
                    document.body.removeChild(svg);

                    if (window && window.requestAnimationFrame) {
                        // In order to work around a Firefox bug (webcompat/web-bugs#119834) we
                        // need to wait one extra frame before it's safe to read the image data.
                        window.requestAnimationFrame(function () {
                            resolve(image);
                        });
                    } else {
                        // If we don't have a window or requestAnimationFrame function proceed immediately.
                        resolve(image);
                    }
                };
                
                image.onerror = (error) => {
                    // Cleanup: remove the image from the document
                    document.body.removeChild(svg);

                    reject(error);
                };

                svg.appendChild(image);
                image.src = uri;
                
                // Add the SVG to the document body (invisible)
                document.body.appendChild(svg);
            });
        }

@IDisposable
Copy link
Member

Drafted a v3.5.0-beta.0 release for you to try.

@ecancil
Copy link

ecancil commented Oct 15, 2024

@IDisposable

I will try out this beta release. For me, this has completely fixed the webkit issues that have plagued (as you can see in reported bugs) users of this (really awesome) library for years now. I really hope this helps folks out.

I had my back up against a wall for a deadline so hadn't made all of the tests pass yet, so appreciate you looking into it.

I will check out the beta release shortly

@ecancil
Copy link

ecancil commented Oct 16, 2024

@IDisposable

this seems to be as effective as what i did on the fly and posted here.

Thanks for entertaining this change.

@ecancil
Copy link

ecancil commented Oct 17, 2024

@IDisposable so does this effectively mean you support WebKit and safari now with the newest release?

@IDisposable
Copy link
Member

I guess it does! That said, I have no ability to test anything in that environment, so it could break down the road (hope not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants