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

Show loading indicator while ViaHTML iframe is loading #465

Merged
merged 5 commits into from
May 4, 2021

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Apr 28, 2021

In Safari there is no indicator that anything is loading once the main
frame has loaded, the user just sees a blank white space where the
iframe is.

To provide a better experience add a loading bar at the top of the page. This is shown until the iframe reports the document metadata to the parent frame for the first time, corresponding
to the DOMContentLoaded event having been received in the iframe. This
is also when the tab title gets set. (Edited: The spinner has been changed to a loading bar)

The loading spinner HTML and SVG have been taken from the
annotation.html.jinja2, bouncer.css and hypothesis-icon.svg files
in the hypothesis/bouncer repository. All of the resources have been
inlined here, which optimizes loading a little, but we could quite
easily move them to external resources in future for maintainability.
(Edit: The spinner was replaced with a red bar.)

Fixes hypothesis/viahtml#155

@seanh
Copy link
Contributor

seanh commented Apr 28, 2021

A couple of issues in testing this:

It fails to hide the spinner when loading a PDF rather than an HTML page. The background colour behind the spinner is also wrong when loading a PDF (it's set by PDF.js):

Peek 2021-04-28 17-20

I think the background colour could also be an issue with HTML pages that set a non-white background.

For HTML pages also I notice that the spinner is still shown for a second on top of the page contents once they become visible:

Peek 2021-04-28 17-19

@seanh
Copy link
Contributor

seanh commented Apr 28, 2021

Instead of showing a spinner in front of the third-party page or PDF, I wonder if a better approach might be to show a loading bar above it? Either at the very top of the iframe, or above the iframe. Some sites show a simple blue line, a few pixels high, at the top of the page that moves from left to right as the page loads. The advantage of this is no clashing between our spinner and the site or PDF behind it. GitHub does this for example:

Peek 2021-04-28 17-24

The problem of course is that using a progress bar rather than a spinner requires the code to have some at least rough idea of how much of the site has loaded, not just that it's still loading. There are bar-shaped loading indicators that simply use animation to indicate that "something is happening" without indicating how much progress has been made, this sort of thing:

Peek 2021-04-28 17-26

@robertknight
Copy link
Member Author

I have pushed to changes to address the issues above:

  • The loading indicator no longer lingers in the PDF viewer. The fix is to make the PDF viewer iframe send the same document metadata message to the parent that a ViaHTML frame does. This also fixes the tab title in the PDF viewer.
  • The display of the loading indicator has been delayed by .5s so that it doesn't flicker briefly if the content loads quickly
  • The loading indicator now fades in and out to reduce the flicker effect

Regarding the loading indicator style, I considered a GitHub-style progress bar but I thought this might be too hidden on the initial page load. On GitHub this indicator is shown when transitioning from one GH page to another so there isn't the "blank slate" problem on first load. I think we'd have to try some different variants to be sure.

@robertknight
Copy link
Member Author

robertknight commented Apr 29, 2021

An issue that is not addressed here is what to do if the Via 3 request succeeds and renders the wrapper page containing an iframe, but the ViaHTML page fails to load for any reason or renders a page without the banner_insert.html header. From what I can find so far, there is no event that gets emitted if the iframe fails to load for any reason. If that is the case then the best we can do is to add a timeout.

Edit: Related issue about this: whatwg/html#125. Our use case is very similar to the ones in the comments that mention using postMessage to communicate with a parent frame.

@seanh
Copy link
Contributor

seanh commented Apr 29, 2021

Works with PDFs now 👍

There are examples of the "bar at the top of the frame" design being used in initial page loads. PDF.js does it for example (sorry long/slow GIF, wait for it):

Peek 2021-04-29 17-44

The problem is that, in Safari, the user can be faced with a blank white window and nothing happening, right? If the only thing in the blank white window is a loading bar at the top, the user will notice it.

I really think that showing a spinner with transparent elements over arbitrary third-party content is a problem. It creates visual clashing and legibility issues that will vary depending on the third-party site that's behind it, and it just sort of looks like broken CSS. Example:

Screenshot from 2021-04-29 17-39-49

Here's what it'll look like if the third-party site uses the same colour as we do:

Screenshot from 2021-04-29 17-47-42

@seanh
Copy link
Contributor

seanh commented Apr 29, 2021

Maybe put another way: there's a reason why browsers show a tiny loading spinner up on the tab (where they have control of the background colour and surroundings), rather than a big one in the middle of the window

@seanh
Copy link
Contributor

seanh commented Apr 29, 2021

@robertknight Do you want to push ahead with this design, or try out some variants? And do you want to do anything about the case when the iframe's contents fail to load?

In Safari there is no indicator that anything is loading once the main
frame has loaded, the user just sees a blank white space where the
iframe is.

To provide a better experience add a loading spinner in the center of
the page above the iframe in all browsers. This is shown until the iframe reports the
document metadata to the parent frame for the first time, corresponding
to the `DOMContentLoaded` event having been received in the iframe. This
is also when the tab title gets set.

The loading spinner HTML and SVG have been taken from the
`annotation.html.jinja2`, `bouncer.css` and `hypothesis-icon.svg` files
in the hypothesis/bouncer repository. All of the resources have been
inlined here, which optimizes loading a little, but we could quite
easily move them to external resources in future for maintainability.

Fixes hypothesis/viahtml#155
When visiting `http://<VIA_DOMAIN>/<PDF_URL>` then the PDF viewer is
displayed inside an iframe. In order for the top frame rendered by the
`proxy` view to reflect the document title and PDF URL the PDF viewer
iframe needs to send the same `metadatachange` message that ViaHTML
sends.

The "real" PDF title is not available until the document has loaded, and
is often incorrect anyway. Therefore we send the filename of the PDF
extracted from the URL if available, or the domain from which the PDF
was loaded otherwise.
 - Wait for .5s before showing the spinner, to avoid an unnecessary
   flash if the content loads quickly

 - Fade the spinner in and out over 200ms to reduce the flicker effect

 - Change the class name from `js-loading-spinner` to a more generic
   `js-loading-indicator` in case we decide to change the indicator type
   in future (eg. from a spinner to a progress bar)
Replace the loading spinner in the center of the screen with a thin red
bar at the top of the screen. This indicator is modeled after the one used by
GitHub (a thin blue bar) and PDF.js (a slightly thicker white bar).
@robertknight robertknight changed the title Show loading spinner while ViaHTML iframe is loading Show loading indicator while ViaHTML iframe is loading May 4, 2021
@robertknight
Copy link
Member Author

robertknight commented May 4, 2021

I have changed the loading spinner to a solid bar that is similar to GitHub's, but using our brand color instead of blue and is slightly thicker to make it more visible. This avoids an issue where the spinner can obscure the page content if it takes a long time in between some content appearing and the DOMContentLoaded event being fired in the page. New version:

Loading.bar.animation.mov

As with GitHub's bar, this one doesn't use an indeterminate state during the initial load. Instead it just grows to a fixed progress percentage and stops until loading is "done" at which point it grows to 100%.

To get a sense of what it looks like under different conditions, open Chrome devtools, go to the Network tab and enable connection throttling at various levels.

*/
.loading-bar.is-done {
animation: loading-bar-finish;
animation-duration: 1s;
Copy link
Member Author

Choose a reason for hiding this comment

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

The animation is not actually 1s long as it reaches the final state at 70%. Using 1s makes each 10% of the animation take 100ms, which is convenient.

Copy link
Contributor

@seanh seanh left a comment

Choose a reason for hiding this comment

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

Looks great!

@robertknight robertknight merged commit 868c7a6 into master May 4, 2021
@robertknight robertknight deleted the add-loading-spinner branch May 4, 2021 09:24
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

Successfully merging this pull request may close these issues.

Indicate when the proxied page is loading
2 participants