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

Etherpad and Hypothesis Embed Code (iframe problems) #2244

Closed
JohnMcLear opened this Issue May 19, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@JohnMcLear
Copy link

commented May 19, 2015

I thought I already made an issue for this but I can't find it, I'm going to create another issue here just to document the problem.

The TLDR; is the controls for Hypoteh.is need to be injected into the page with the content and this causes the contents to be written into the pad itself.

The problem: We use iframes to embed pad contents into a page in Etherpad, while this is somewhat hacky it's the current status quo.

The "fix" would to allow the javascript to inject the controls into a target item which in my case would be parent.body.. A syntax might be..

<script async defer src="//hypothes.is/embedTarget.js"></script>
<script>
hypothesis.controlsAppendTarget("parent.body");
</script>

The above targeting would only modify the "append" parts of the hypothesis embed client. All range selection / selection events would still be triggered on the iframe where the pad is loaded.

The "real" fix here and there is no hiding from this is that we need to not use iframes in Etherpad so if that's your answer I'm totally fine with that too.

@judell

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

Inability to work within embedded iframes is a painful limitation for us in general, and telling people not to use iframes is a poor solution. Your suggestion is intriguing!

@JohnMcLear

This comment has been minimized.

Copy link
Author

commented May 19, 2015

Obvious limitation is if the iframe child/parent is on another domain..

@judell

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

I'm also hard-pressed to visualize how scrolling might or might not work in this scenario. If you try an experiment before we do, please let us know!

@JohnMcLear

This comment has been minimized.

Copy link
Author

commented May 19, 2015

Scrolling in Etherpad works by scrolling inner window and outerwindow at the same time.

I can't do an experiment as the current methods only target the body of the page that has loaded the javascript. :(

@judell

This comment has been minimized.

Copy link
Contributor

commented May 19, 2015

OK, thanks. I'll add Etherpad to the growing list of iframe use cases we would like to solve.

@tilgovi

This comment has been minimized.

Copy link
Contributor

commented May 20, 2015

An option for the container to inject into would be great for these simple same-domain scenarios.

@tilgovi

This comment has been minimized.

Copy link
Contributor

commented May 20, 2015

Super simple. 👍

If you want to take a crack at implementing it, see the code with the comment Create the frame in h/static/scripts/annotator/host.coffee.

You can already send additional options into this constructor by returning an object from a function set on window.hypothesisConfig(). Our code calls that first, so you can set your container there.

Something like:

function hypothesisConfig() {
  return {container: parent.contentDocument.body};
}

And then check options.container in host.coffee.

@JohnMcLear

This comment has been minimized.

Copy link
Author

commented Jul 5, 2015

FWIW I tried parent.document.body and this returns an error

Uncaught TypeError: Cannot read property 'module' of undefined
(anonymous function) @ angular-animate.min.js?d4b6a6fb:1
(anonymous function) @ angular-animate.min.js?d4b6a6fb:1

Weirdly document.body returns the same error..

window.document.documentElement does not through any error nor does parent.parent.document.documentElement..

However one would expect parent.parent.document.documentElement would apply the contents to the correct target (two parents above) but in this instance it does not...

FWIW parent.parent.document.body is the correct "target" imho

Looking at https://github.com/hypothesis/h/blob/master/h/static/scripts/annotator/host.coffee#L21 container is never referenced? I assume it is rewritten from container to element or so during options application?

@tilgovi

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2015

There are no scripts for building the front-end code directly right now, however @BigBlueHat and I have been talking about this issue quite a bit lately.

Building the CSS and the JS should not be too much work. The definitions of the bundles are in h/assets.yaml and one could use compass and browserify directly to build similar bundles. However, the templates need some work to disentangle the front end and back end templating, particularly the app.html template that provides the base markup for the sidebar.

I'm not sure what the error is that you report when trying to change the container for the sidebar is, but when I looked into it just now I realized you're right about not actually using container. If you'd like to add support for that option that would be excellent. Open a PR to use options.container as the argument of the appendTo on line 30 and add something to the documentation in docs/hacking/customized-embedding.rst and I'll merge it straight away.

Thanks, @JohnMcLear!

@nickstenning

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

Hi there! I'm going to close this as part of a clean-up of all issues currently open on this repo that represent ideas or features rather than reports of bugs or technical chores.

I want to be clear that this isn't intended to say anything at all about the content of this issue—it certainly doesn't mean we're no longer planning to do the work discussed here—just that we don't want to use GitHub issues to track feature requests or ideas, because the threads can get long and somewhat unwieldy.

If you're interested in what we are working on at the moment, you can check out our Trello board and, for a longer-term view, our roadmap.

And, if you're interested in following up on this issue, please do continue the discussion on our developer community mailing list. You might also want to check out our contributing guide.

@JohnMcLear

This comment has been minimized.

Copy link
Author

commented Feb 10, 2016

FWIW we ended up doing our own comment implementation, https://github.com/JohnMcLear/ep_comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.