Add front page and legacy Via URL support #443

seanh · 2021-04-16T10:54:36Z

See #424
TODO:

The front page needs to be optional. LMS's Via shouldn't have one
Add options to disable the LMS lockdown: Add Via 3 & Via HTML options to disable the LMS-only lockdown checkmate#99
Make sure that the via.hypothes.is URL isn't used as the annotation's target URL (for either PDFs or HTML) (this needs to be done before we can release this to users but I'll probably create a separate issue for it, we can deploy and test this PR without fixing this)

Known issues:

The URL in the location bar won't change if the iframe is navigated. For example if the third-party URL returns a redirect. I think this is different from how legacy Via behaves. But I'm not completely sure it's actually a problem. I might propose to open an issue to track this but not fix it unless we decide we have to.

In theory I think this is fixable by adding a bit of JavaScript to either poll or get notified of changes to the iframe's URL and push new URLs onto the location bar without triggering page reloads.

seanh · 2021-04-16T16:49:32Z

via/templates/proxy.html.jinja2

+    body {
+      margin: 0;
+      padding: 0;
+    }
+
+    .iframe {
+      position:fixed;
+      top:0;
+      left:0;
+      bottom:0;
+      right:0;
+      width:100%;
+      height:100%;
+      border:none;
+      margin:0;
+      padding:0;
+      overflow:hidden;
+      z-index:999999;
+    }


@robertknight @lyzadanger What's the right way to create an invisible "full-screen" iframe? This seems to work

seanh · 2021-04-16T16:50:12Z

tests/functional/static_content_test.py

-    def test_get_front_page(self, test_app):
-        response = test_app.get("/", status=404)
-
-        assert dict(response.headers) == Any.dict.containing(
-            {"Content-Type": Any.string.containing("text/plain")}
-        )


The front page is no longer a 404. Also it's not served as static content anymore

seanh · 2021-04-16T16:51:42Z

via/templates/index.html.jinja2

+  <style>
+    body {
+      font-family: "Helvetica Neue", Helvetica, Arial, "Lucida Grande", sans-serif;
+      height: 100vh;
+      padding: 0;
+      margin: 0;
+    }
+
+    .content {
+      align-items: center;
+      display: flex;
+      flex-direction: column;
+      height: 100%;
+      justify-content: center;
+      max-height: 70vh;
+    }
+
+    .url-form {
+      align-items: center;
+      display: flex;
+      flex-direction: column;
+      max-width: 400px;
+      width: 95%;
+    }
+
+    /* Input field for URL to annotate. */
+    .url-field {
+      align-self: stretch;
+      border-radius: 3px;
+      border: 1px solid #ccc;
+      font-size: 20px;
+      margin-bottom: 20px;
+      margin-top: 20px;
+      min-height: 45px;
+      padding-bottom: 5px;
+      padding-left: 16px;
+      padding-right: 16px;
+      padding-top: 5px;
+    }
+
+    .url-field:focus {
+      border-color: #3f3f3f;
+    }
+
+    /* Button that takes the user to the proxied page. */
+    .annotate-btn {
+      background-color: #3f3f3f;
+      border-radius: 3px;
+      border: none;
+      color: #fff;
+      cursor: pointer;
+      font-size: 20px;
+      font-weight: 700;
+      padding-bottom: 10px;
+      padding-left: 15px;
+      padding-right: 15px;
+      padding-top: 10px;
+    }
+
+    .annotate-btn:hover {
+      background-color: #d00032;
+    }
+  </style>


This CSS is all as-in legacy Via

seanh · 2021-04-16T16:51:56Z

via/templates/index.html.jinja2

+    <a href="https://hypothes.is">
+      <svg xmlns="http://www.w3.org/2000/svg"
+        width="100"
+        height="100"
+        viewBox="0 0 24 28"
+        aria-labelledby="logo-title">
+        <title id="logo-title">Hypothesis homepage</title>
+        <g fill="none" fill-rule="evenodd"><path fill="#FFF" fill-rule="nonzero" d="M3.886 3.945H21.03v16.047H3.886z"/><path d="M0 2.005C0 .898.897 0 2.005 0h19.99C23.102 0 24 .897 24 2.005v19.99A2.005 2.005 0 0 1 21.995 24H2.005A2.005 2.005 0 0 1 0 21.995V2.005zM9 24h6l-3 4-3-4zM7.008 4H4v16h3.008v-4.997C7.008 12.005 8.168 12.01 9 12c1 .007 2.019.06 2.019 2.003V20h3.008v-6.891C14.027 10 12 9.003 10 9.003c-1.99 0-2 0-2.992 1.999V4zM19 19.987c1.105 0 2-.893 2-1.994A1.997 1.997 0 0 0 19 16c-1.105 0-2 .892-2 1.993s.895 1.994 2 1.994z" fill="#3F3F3F"/></g>
+      </svg>
+    </a>


Logo as in legacy Via

seanh · 2021-04-16T16:52:14Z

via/templates/index.html.jinja2

+      <p>
+        Via is a closed proxy that lets you annotate web pages and PDFs with
+        Hypothesis. Only a small number of known sites can be annotated. If you'd
+        like to annotate a site that isn't allowed please
+        <a href="https://web.hypothes.is/via-request/">ask us to add it to the allowed list.</a>
+        See our
+        <a href="https://web.hypothes.is/help/what-is-the-via-proxy/">help article</a>
+        for more information.
+      </p>


Same exact notice as from legacy Via

seanh · 2021-04-16T16:52:46Z

via/views/exceptions.py

-    renderer="via:templates/exception.html.jinja2",
-    # Match any page *except* the front page ("/").
-    # We don't want our fancy error page to be used on the front page.
-    path_info=not_(r"^\/$"),


We actually do want to apply the exception view to the front page now

Can you clarify when this would be used in the context of the front page?

Actually I don't think that it can be used, since the views for the front page never raise anything, but the the exception was put here originally when we removed Via 3's front page (to hide Via 3 from abusers because we didn't want the resulting 404 for via3.hypothes.is requests to cause the fancy error page to kick in. Now that we're putting the front page back it's no longer needed

seanh · 2021-04-16T17:04:58Z

via/views/proxy.py

+@view_config(route_name="proxy", renderer="via:templates/proxy.html.jinja2")
+def proxy(request):
+    url = request.path[1:]  # Strip the leading "/" from the path.
+
+    # Let people just type in "example.com" or link to
+    # via.hypothes.is/example.com and have that proxy https://example.com.
+    if not (url.startswith("http://") or url.startswith("https://")):
+        url = "https://" + url
+
+    return {"src": request.route_url("route_by_content", _query={"url": url})}


This is the view that implements the legacy Via-style URLs. It accepts requests to via.hypothes.is/<URL_TO_BE_PROXIED> and renders a page containing an iframe whose src is via.hypothes.is/route?url=<URL_TO_BE_PROXIED>. That iframe will then get redirect to either /pdf or to Via HTML as appropriate, but the user isn't seeing the iframe's URL in the location bar so the /route, /pdf and Via HTML URLs are hidden from the user

It would be nice to save that extra redirect from the flow, since that involves an extra roundtrip between the user's browser and Via.

Maybe caching?

Actually, the new view can also do the same PDF-or-HTML check that the /route view does and inject the /pdf or Via HTML URL directly, so we should be able to remove the extra round trip

Made an issue for this #451

seanh · 2021-04-16T17:05:28Z

via/views/index.py

+    @staticmethod
+    @view_config(request_method="GET", renderer="via:templates/index.html.jinja2")
+    def get():
+        return {}


The front page. Renders the form where you can enter a URL that you want to annotate

seanh · 2021-04-16T17:06:31Z

via/views/index.py

+    @view_config(request_method="POST")
+    def post(self):
+        return HTTPFound(
+            self.request.route_url(
+                route_name="proxy", url=self.request.params.get("url", "")
+            )
+        )


This is the view for POST requests to the front page. Accepts form submissions and redirects to the legacy Via-style URL for proxying the URL that was submitted in the form.

If there is no url param or if it's an empty string this ends up redirecting back to the front page.

If url isn't a valid URL that'll trigger the existing nice error page when we try to proxy it. There's no need for this view to validate URLs.

robertknight · 2021-04-19T10:50:03Z

Make sure that the via.hypothes.is URL isn't used as the annotation's target URL (for either PDFs or HTML)

For PDFs this was addressed by #427. For HTML documents this is handled by a <link rel="canonical"> in the page. See https://github.com/hypothesis/viahtml/blob/eee5fb4a6bbc59cdd7ba1066ad350ff0c10fb3cd/viahtml/templates/banner.html#L2.

robertknight · 2021-04-19T10:55:04Z

In theory I think this is fixable by adding a bit of JavaScript to either poll or get notified of changes to the iframe's URL and push new URLs onto the location bar without triggering page reloads.

Yes, something like the following in the viahtml iframe:

// Is this a top-level iframe embedded inside Via?
if (window.top !== window && window.top === window.parent) {
  window.top.postMessage({
    type: 'via:urlChange',
    url: window.location.href,
  }, '*');
}

And then corresponding code in the Via 3 frame that uses the history API to update the URL.

The most obvious and immediate deficiency from a user's point of view though is that the document title is not set. We could notify Via about that in a similar way.

robertknight

LGTM with some minor feedback and queries on the CSS.

From the user's perspective the most obvious issue with the iframe approach from initial testing is that the proxied document's title, favicon and loading status are not reflected in the tab state. Reflecting basic document metadata (or some variant of it) is something that can be solved. I'm not sure about the loading status. We might want to consider displaying some kind of placeholder in the blank frame until it has loaded.

robertknight · 2021-04-19T11:00:13Z

via/templates/index.html.jinja2

+  <meta charset="utf-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <title>via.hypothes.is</title>
+  <link rel="icon" href="favicon.ico" type="image/x-icon" />


If we're going the iframe route I do think we'll need to work on updating at least some of the parent page metadata (eg. title) to make tabs distinguishable from one another. A bonus here is that we can easily add some indicator that the frame is being proxied through Via to make it easily distinguishable from the original page in bookmarks etc.

Oh yeah, that's nice. I think we probably actually want the favicon to be the Hypothesis one not the third-party one. But we could set the tab title to something like Via - <title of third-party page> (or perhaps Hypothesis - ?)

Made an issue for this: hypothesis/viahtml#154

Note that I don't think we want the favicon to change (even though it does with legacy Via)

robertknight · 2021-04-19T11:02:03Z

via/templates/index.html.jinja2

+             autofocus
+             class="url-field"
+             name="url"
+             placeholder="Paste a link to annotate"


Legacy Via has a slightly different placeholder which reads "Paste a link to create or view annotations".

Yes I know, I thought I'd sneak this change in :) "Create or view annotations" is pretty awkward, typical Hypothesis-speak. Maybe it should say "Create or view annotations, highlights and page notes" :)

robertknight · 2021-04-19T11:02:38Z

via/templates/proxy.html.jinja2

+      margin: 0;
+      padding: 0;
+      overflow: hidden;
+      z-index: 999999;


What is the z-index for? There is nothing that the iframe needs to be positioned on top of.

Not sure, it doesn't seem to be necessary so I'll try removing it

robertknight · 2021-04-19T11:03:27Z

via/templates/proxy.html.jinja2

+      border: none;
+      margin: 0;
+      padding: 0;
+      overflow: hidden;


What is overflow for? An iframe can't overflow its container AFAIK so there is no need for that.

Again not sure, I'll try removing it

robertknight · 2021-04-19T11:04:14Z

via/templates/proxy.html.jinja2

+      padding: 0;
+    }
+
+    .iframe {


Can you give this class a more meaningful name. Say proxied-content-iframe.

robertknight · 2021-04-19T11:05:06Z

via/views/exceptions.py

-    renderer="via:templates/exception.html.jinja2",
-    # Match any page *except* the front page ("/").
-    # We don't want our fancy error page to be used on the front page.
-    path_info=not_(r"^\/$"),


Can you clarify when this would be used in the context of the front page?

robertknight · 2021-04-19T11:08:48Z

via/views/proxy.py

+    # Let people just type in "example.com" or link to
+    # via.hypothes.is/example.com and have that proxy https://example.com.
+    if not (url.startswith("http://") or url.startswith("https://")):
+        url = "https://" + url


IIRC legacy via used to default to http:// but I think https:// is the right choice in the modern era. We'll have to keep an eye out for this.

robertknight · 2021-04-19T11:10:00Z

via/views/proxy.py

+@view_config(route_name="proxy", renderer="via:templates/proxy.html.jinja2")
+def proxy(request):
+    url = request.path[1:]  # Strip the leading "/" from the path.
+
+    # Let people just type in "example.com" or link to
+    # via.hypothes.is/example.com and have that proxy https://example.com.
+    if not (url.startswith("http://") or url.startswith("https://")):
+        url = "https://" + url
+
+    return {"src": request.route_url("route_by_content", _query={"url": url})}


It would be nice to save that extra redirect from the flow, since that involves an extra roundtrip between the user's browser and Via.

seanh · 2021-04-19T11:19:28Z

We might want to consider displaying some kind of placeholder in the blank frame until it has loaded.

That might actually be quite a nice UX, as it can signal to the user that via.hypothes.is has loaded and they're now waiting for example.com to load/proxy. Created an issue for this: hypothesis/viahtml#155

seanh · 2021-04-19T13:26:57Z

Make sure that the via.hypothes.is URL isn't used as the annotation's target URL (for either PDFs or HTML)

For PDFs this was addressed by #427. For HTML documents this is handled by a <link rel="canonical"> in the page. See https://github.com/hypothesis/viahtml/blob/eee5fb4a6bbc59cdd7ba1066ad350ff0c10fb3cd/viahtml/templates/banner.html#L2.

Ok, sounds like this is handled for both Via 3 and Via HTML, so nothing to do.

seanh · 2021-04-19T13:42:14Z

Known issues:

The URL in the location bar won't change if the iframe is navigated. For example if the third-party URL returns a redirect. I think this is different from how legacy Via behaves. But I'm not completely sure it's actually a problem. I might propose to open an issue to track this but not fix it unless we decide we have to.

In theory I think this is fixable by adding a bit of JavaScript to either poll or get notified of changes to the iframe's URL and push new URLs onto the location bar without triggering page reloads.

Opened an issue for this: hypothesis/viahtml#156

seanh force-pushed the add-front-page branch from ca4e2e8 to 9ae589c Compare April 16, 2021 16:46

seanh commented Apr 16, 2021

View reviewed changes

seanh changed the title ~~Add front page~~ Add front page and legacy Via URL support Apr 16, 2021

seanh marked this pull request as ready for review April 16, 2021 17:13

seanh requested a review from robertknight April 16, 2021 17:13

seanh force-pushed the add-front-page branch from 9ae589c to 25f88b6 Compare April 16, 2021 17:14

seanh mentioned this pull request Apr 19, 2021

Finish, deploy & test the iframe-based proof-of-concept for replacing legacy Via #424

Closed

robertknight approved these changes Apr 19, 2021

View reviewed changes

seanh mentioned this pull request Apr 19, 2021

Avoid the additional network round trip for legacy Via-style URLs #451

Closed

seanh mentioned this pull request Apr 19, 2021

Indicate when the proxied page is loading hypothesis/viahtml#155

Closed

seanh force-pushed the add-front-page branch from 25f88b6 to 77e0902 Compare April 19, 2021 13:45

seanh added 2 commits April 19, 2021 15:00

Add a front page and legacy Via URL support

0be8ea6

Optionally disable the front page

1842365

seanh force-pushed the add-front-page branch from 77e0902 to 1842365 Compare April 19, 2021 14:00

seanh merged commit 7c73a2c into master Apr 19, 2021

seanh deleted the add-front-page branch April 19, 2021 14:06

Add front page and legacy Via URL support #443

Add front page and legacy Via URL support #443

Conversation

seanh commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

seanh Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Apr 19, 2021

robertknight commented Apr 19, 2021 • edited Loading

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Apr 19, 2021 • edited Loading

seanh commented Apr 19, 2021

seanh commented Apr 19, 2021

seanh commented Apr 16, 2021 •

edited

Loading

seanh Apr 19, 2021 •

edited

Loading

seanh Apr 16, 2021 •

edited

Loading

robertknight commented Apr 19, 2021 •

edited

Loading

seanh commented Apr 19, 2021 •

edited

Loading