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

bind:clientWidth doesn't work in sandbox #2147

Closed
Rich-Harris opened this issue Mar 2, 2019 · 13 comments · Fixed by #2989
Closed

bind:clientWidth doesn't work in sandbox #2147

Rich-Harris opened this issue Mar 2, 2019 · 13 comments · Fixed by #2989

Comments

@Rich-Harris
Copy link
Member

REPL. Resizing the viewer window doesn't change anything.

@Rich-Harris Rich-Harris added the bug label Mar 2, 2019
@Rich-Harris Rich-Harris added this to the 3.0 milestone Mar 2, 2019
@Panya
Copy link
Contributor

Panya commented Mar 10, 2019

I believe it's repl only issue. Seems like contentDocument of the created object element (which used for resize handler) is null if used with eval. The same issue exists in v2 (REPL).

update: I've tested the example in a Sapper app and all work as expected.

@Rich-Harris
Copy link
Member Author

Yeah, this is tricky. There's no way to use the element resize listener technique this binding uses inside an iframe that doesn't have both allow-scripts and allow-same-origin sandbox properties, which is strongly discouraged (since it basically allows the sandbox to be completely bypassed).

Options:

  • Relax sandbox restrictions and hope that no-one creates an app that maliciously uses other people's GitHub credentials to modify gists
  • Have broken tutorials
  • Change the behaviour of the binding so that it polls for changes
  • Use ResizeObserver instead (t only works on Chrome)
  • Serve the REPL on a different domain and come up with some convoluted message-passing mechanism (I think this is what CodeSandbox is doing? This v2 example works)
  • Add some UI that allows the user to opt in to a more relaxed sandbox, if they trust the app

All these options are terrible. The last one is probably the least terrible. Am I missing any?

@halfnelson
Copy link
Contributor

Below works on firefox, but is a bit heavy.

let resizeListenerId = 0;
function addResizeListener(element, fn) {
    if (getComputedStyle(element).position === 'static') {
        element.style.position = 'relative';
    }

    const object = document.createElement('object');
    object.setAttribute('style', 'display: block; position: absolute; top: 0; left: 0; height: 100%; width: 100%; overflow: hidden; pointer-events: none; z-index: -1;');
    object.type = 'text/html';

    let win;
    let resizeMessageKey = `resized-${resizeListenerId++}`
    let eventProxyScript = "data:text/html;charset=utf-8,"+ encodeURIComponent(
        `<script>window.addEventListener('resize',  () => window.parent.postMessage("${resizeMessageKey}",'*'))</script>`
    );
    let messageHandler
    
    object.onload = () => {
        if (window.origin == 'null') {
            messageHandler = (e) => { if (e.data == resizeMessageKey) fn() }
            window.addEventListener('message', messageHandler );
        } else {
            win = object.contentDocument.defaultView;
            win.addEventListener('resize', fn);
        }
        
    };

    if (/Trident/.test(navigator.userAgent)) {
        element.appendChild(object);
        object.data = window.origin == 'null' ? eventProxyScript : 'about:blank';
    } else {
        object.data = window.origin == 'null' ? eventProxyScript : 'about:blank';
        element.appendChild(object);
    }

    return {
        cancel: () => {
            if (win && win.removeEventListener) {
                win.removeEventListener('resize', fn);
            } 
            if (messageHandler) {
                window.removeEventListener('message', messageHandler);
            }
            element.removeChild(object);
        }
    };
}

@arxpoetica
Copy link
Member

Set an alert of some kind that asks the user if they want to refresh?

@Conduitry
Copy link
Member

Conduitry commented Mar 20, 2019

How practical would it be to have the sandbox default to being more relaxed just on tutorials, since the only code getting run there would be stuff we write or that the user writes or pastes in. We could still keep the more restrictive settings on the main REPL, possibly with a button to relax them. Or we could even have the REPL default to relaxed if you visit it via one of the built-in examples, and only put it in the more restrictive mode upon loading a Gist.

@Rich-Harris
Copy link
Member Author

Actually you know what? That's a great idea. Certainly in the short term — I guess it'd be a little confusing if you forked one of the examples and it stopped working.

We could also relax the sandbox if you're on your own gist, maybe. And perhaps we could detect the existence of non-sandbox-friendly things (if you're on someone else's gist) and display a warning in those cases?

Rich-Harris added a commit that referenced this issue Mar 20, 2019
run tutorial and widget REPLs in relaxed sandbox
@Rich-Harris
Copy link
Member Author

This now works with @Conduitry's fix: https://v3.svelte.technology/tutorial/dimensions

Will leave this open so we can explore the more complete solutions later

@Rich-Harris Rich-Harris modified the milestones: 3.0, 3.x Mar 21, 2019
@Conduitry Conduitry changed the title bind:clientWidth doesn't work bind:clientWidth doesn't work in sandbox Apr 15, 2019
@hillar
Copy link

hillar commented Sep 17, 2019

I guess it'd be a little confusing
mhmh, it was ... and and it took a lot time ;(

can you please write with big red letters, that it works, but just does not work in svelte.dev/repl

@kewp
Copy link

kewp commented Apr 15, 2020

I'm getting the same error on my app in iOS Safari.

null is not an object (evaluating 'r.contentDocument.defaultView')

which is happening on line 352 of index.mjs (also inside add_resize_listener)

351.   object.onload = () => {
352.       win = object.contentDocument.defaultView;
353.       win.addEventListener('resize', fn);
354.    };

I'm using Sentry to track error occurances so I'm just getting a report from them. Apparently this is all from Safari, and mostly iOS. I can't seem to reproduce this locally though (even using the same iOS / Safari version).

@Conduitry
Copy link
Member

This works in 3.21.0, finally! https://svelte.dev/repl/648026c694e2323428e82b60c38829d0?version=3.21.0 Thanks @mrkishi!

@philholden
Copy link

How about using resizeObserver if available and the iFrame trick only as a fallback?
It is now supported on Edge, Chrome, Firefox, Safari.

https://caniuse.com/#feat=resizeobserver

The metics it provides may not be helpful (most browsers only provide the contentRect when you normally want borderBox). But it should be a lighter weight way of detecting that a resize has happened.

@surjithctly
Copy link

Just got the same error. Which means the bug exists.

image

I somehow thought its my error until I find this thread. It would be great if the message conveys that its a REPL issue.

@iltimasd
Copy link

@surjithctly your error does not look related to this issue. You are using querySelector before the component has mounted. If that function in used in an onMount call, then your error disappears.

https://svelte.dev/repl/45446e457d9f43c09e2eebe401955917?version=3.29.7

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

Successfully merging a pull request may close this issue.

10 participants