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

Workaround Chromium issue with iframe reload/href #10185

Merged
merged 2 commits into from May 10, 2021

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented May 3, 2021

References

Fixes #10121

Code changes

Remove reload iframe logic. For the original reason for re-loading see: #3399. The tests with modern browsers indicate that it is no longer necessary (and leads to issues with SVG files in Chrome).

User-facing changes

Backwards-incompatible changes

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@krassowski krassowski marked this pull request as draft May 3, 2021 18:23
@krassowski
Copy link
Member Author

Should not break:

from IPython.display import display_svg, SVG
display_svg(SVG(url='http://upload.wikimedia.org/wikipedia/en/a/a4/Flag_of_the_United_States.svg'), metadata=dict(isolated=True))

Should fix:

from IPython.display import HTML
content = HTML("""<a>hi</a>""")
display(content, metadata={'isolated': True})

@krassowski
Copy link
Member Author

It always evaluates to false as it seems though.

@blink1073 blink1073 added this to the 3.1 milestone May 3, 2021
@blink1073 blink1073 added the bug label May 3, 2021
@krassowski krassowski marked this pull request as ready for review May 5, 2021 16:14
Copy link
Contributor

@agoose77 agoose77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @krassowski, great work.

Would you mind modifying the PR description to reflect the new fix; i.e.

Remove iframe reload logic

or something?

Otherwise, if this can't be reproduced on any modern browsers then it might be fine? I'm a bit concerned about causing a regression in slightly older clients, but I don't know what our policy on that is, so I defer to those that who do!

@krassowski
Copy link
Member Author

krassowski commented May 7, 2021

Would you mind modifying the PR description to reflect the new fix; i.e.

Updated.

Otherwise, if this can't be reproduced on any modern browsers then it might be fine? I'm a bit concerned about causing a regression in slightly older clients, but I don't know what our policy on that is, so I defer to those that who do!

Yes, it is a tricky problem. I do not know for certain whether the reload was ever strictly necessary as the old PR that introduced it was not atomic (it included other changes too). Though it is probably more important to have a working implementation on current (=secure) browsers rather than stick to what only worked for the old ones.

Edit: But of course if we get a confirmation that it breaks a relatively recent version of a browser that is still in the LTS then we can add a condition on the version of that browser.

@agoose77
Copy link
Contributor

agoose77 commented May 7, 2021

Perhaps that is the solution then; potentially break for some users, and patch their browsers as required!

@krassowski
Copy link
Member Author

@blink1073 sorry for fussing about it, but it would be really great to have it backported to 3.0.x at some point, as I am now stuck without convenient way of generating SVG plots - which is a pain for my day job. Can I help in any way to move it forward?

@blink1073
Copy link
Member

You caught me just in time, I'll work this into releases today.

@blink1073 blink1073 merged commit dc67269 into jupyterlab:master May 10, 2021
@blink1073
Copy link
Member

@meeseeksdev please backport to 3.0.x

@krassowski
Copy link
Member Author

Thanks!

@krassowski krassowski deleted the gh1021-workaround branch May 10, 2021 13:59
blink1073 pushed a commit that referenced this pull request May 10, 2021
…#10223)

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
cameron-toy pushed a commit to cameron-toy/jupyterlab that referenced this pull request May 28, 2021
* Workaround Chromium issue with iframe reload/href

* Try to remove iframe reload altogether
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Nov 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug pkg:outputarea status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsolatedRenderer iframe loads JupyterLab UI in Google Chrome
3 participants