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

Work around Chrome bug causing sidebar to become invisible #523

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

robertknight
Copy link
Member

Work around a Chrome bug [1] that can cause the sidebar to become
invisible if:

  1. The sidebar app is loaded from a Chrome extension AND
  2. The current tab was opened by clicking a link inside the sidebar
    app in a different tab.

When the issue occurs, the sidebar web app loads and runs normally but
is just not visible on screen. Temporarily hiding and showing the iframe
resolves the problem. At the point when the temporary hide & show
occurs, the iframe is positioned to the right of the viewport, so the
user does not see the visibility change.

Fixes #516

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=753314

@codecov
Copy link

codecov bot commented Aug 8, 2017

Codecov Report

Merging #523 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   90.92%   90.93%   +0.01%     
==========================================
  Files         135      136       +1     
  Lines        5419     5426       +7     
  Branches      943      945       +2     
==========================================
+ Hits         4927     4934       +7     
  Misses        492      492
Impacted Files Coverage Δ
.../sidebar/util/disable-opener-for-external-links.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 973bc4d...325741f. Read the comment docs.

@dwhly
Copy link
Member

dwhly commented Aug 9, 2017

Turns out it's an older Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=638375

@robertknight
Copy link
Member Author

From the bug report above, it turns out that there is an alternative and probably better way to fix the problem: Make sure that all links in the sidebar disown their opener when clicked (by adding rel="noopener" to it or intercepting the link click and performing the same action dynamically).

This enables Chrome to use a separate process for the instance of Hypothesis in the newly opened tab, because that Hypothesis instance no longer has synchronous access to the Hypothesis sidebar in the opening tab (via window.top.opener). According to the Chrome devs in that bug report, this should fix the problem in all cases.

This method might be better because it avoids the potential overheads that the display switching might toggle or potential unfortunate interactions between the timeout added here and other parts of the app.

@seanh
Copy link
Contributor

seanh commented Sep 4, 2017

@robertknight rel="noopener" sounds better to me, too. It seems like it would be a security improvement as well - when the user clicks on a (possibly user-created) link in the sidebar and it opens in a new tab, I don't think we want the JavaScript running on that arbitrary third-party site in the new tab to have access to our client's window. I believe this could be used to, for example, change the URL of our iframe back in the original tab. (iirc this issue has been fixed in current versions of some browsers, but still)

@robertknight robertknight force-pushed the chrome-iframe-workaround branch from 9c6e37a to af4838e Compare September 4, 2017 16:00
@robertknight
Copy link
Member Author

@seanh - Thanks for the feedback, I've revised the PR accordingly. Adding rel="noopener" to every link in the client manually would be a painful process, so I've opted to add a piece of code that does it just before a link click is handled, if that link would open in a new tab/window (triggered by having target="_blank").

@robertknight robertknight requested a review from seanh September 4, 2017 16:08
Work around a Chrome bug [1] that can cause the sidebar to become
invisible if:

 1. The sidebar app is loaded from a Chrome extension AND
 2. The current tab was opened by clicking a link inside the sidebar
    app in a different tab.

When the issue occurs, the sidebar web app loads and runs normally but
is just not visible on screen. This happens due to an internal issue in
Chrome which can be avoided adding `rel="noopener"` to all "normal" [2]
links in the client that open URLs in a new tab/window.

Doing so enables Chrome to use a separate process for the Hypothesis
client in the new tab in step (2) than the one used for the Hypothesis
client in step (1). This change also prevents potential tab-jacking
attacks in all browsers that support `rel="noopener"`.

Fixes #516

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=753314
[2] ie. Those which do not use JS to handle the link
@robertknight robertknight force-pushed the chrome-iframe-workaround branch from af4838e to 325741f Compare September 5, 2017 08:26
@seanh seanh self-assigned this Sep 5, 2017
@seanh
Copy link
Contributor

seanh commented Sep 5, 2017

Confirmed that I can still reproduce the issue on master and that this branch fixes it

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!

I also tested that it works for links that are not present on the initial client load (I edited an annotation, adding a direct link to it). Good catch there!

*
* [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types#noopener
* [2] https://mathiasbynens.github.io/rel-noopener/
* [3] https://bugs.chromium.org/p/chromium/issues/detail?id=753314
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice docstring, which is important for a kind of obscure fix like this

@seanh seanh merged commit 92e4974 into master Sep 5, 2017
@seanh seanh deleted the chrome-iframe-workaround branch September 5, 2017 10:35
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.

Transparent sidebar after following a direct link in Chrome v60-61
3 participants