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

Fix Safari multiple tabs by working around a Safari bug. #7316

Merged
merged 1 commit into from Oct 11, 2019

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 10, 2019

References

Fixes #6921

Code changes

We use local storage events to communicate between browser tabs in order to see if there are any other tabs with a particular workspace name. The standard says that local storage events are only supposed to be triggered by local storage changes from other tabs. However, Safari sometimes triggers these events from changes by the current tab. Here is an example illustrating this:

<html>
  <body>
    <div id='report'>No events received</div>
    <script>
      let date = new Date().getTime();
      let report = document.getElementById('report');
      window.addEventListener('storage', ev => {
        if (ev.key !== 'SOME KEY') {
          return;
        }
        if (ev.newValue === date.toString()) {
          report.innerText = `BUG: received my own local storage change (${date}) as an event.`;
        } else {
          report.innerText = `Another tab was reloaded with timestamp ${ev.newValue}`;
        }
      });
      window.localStorage.setItem('SOME KEY', date);
        </script>
  </body>
</html>

Open that page in two different Safari tabs and start refreshing each one alternately. It was pretty easy for me to get one of the tabs to indicate the bug was happening. Things worked fine in firefox and presumably would in Chrome too.

A workaround implemented here is to manually ignore local storage events that we triggered.

Even if we move to BroadcastChannel (see #7315), we'll still need to deal with this since Safari doesn't implement broadcast channel, so we'd have to fall back to something like local storage on Safari.

User-facing changes

Backwards-incompatible changes

None

Fixes jupyterlab#6921

We use local storage events to communicate between browser tabs in order to see if there are any other tabs with a particular workspace name. The standard says that local storage events are only supposed to be triggered by local storage changes from *other* tabs. However, Safari sometimes triggers these events from changes by the current tab. Here is an example illustrating this:

```html
<html>
  <body>
    <div id='report'>No events received</div>
    <script>
      let date = new Date().getTime();
      let report = document.getElementById('report');
      window.addEventListener('storage', ev => {
        if (ev.key !== 'SOME KEY') {
          return;
        }
        if (ev.newValue === date.toString()) {
          report.innerText = `BUG: received my own local storage change (${date}) as an event.`;
        } else {
          report.innerText = `Another tab was reloaded with timestamp ${ev.newValue}`;
        }
      });
      window.localStorage.setItem('SOME KEY', date);
        </script>
  </body>
</html>
```

Open that page in two different Safari tabs and start refreshing each one alternately. It was pretty easy for me to get one of the tabs to indicate the bug was happening. Things worked fine in firefox and presumably would in Chrome too.

A workaround implemented here is to manually ignore local storage events that we triggered.

Even if we move to BroadcastChannel (see jupyterlab#7315), we'll still need to deal with this since Safari doesn't implement broadcast channel, so we'd have to fall back to something like local storage on Safari.
@jasongrout jasongrout added this to the 2.0 milestone Oct 10, 2019
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Oct 10, 2019

Thanks for making a pull request to JupyterLab!

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

@jasongrout jasongrout added tag:Backwards compatible tag:Backport labels Oct 10, 2019
Copy link
Member

@blink1073 blink1073 left a comment

Thanks!

@jasongrout jasongrout merged commit 280212f into jupyterlab:master Oct 11, 2019
9 checks passed
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 11, 2019

@meeseeksdev backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Oct 11, 2019
jasongrout added a commit that referenced this issue Oct 11, 2019
…6-on-1.x

Backport PR #7316 on branch 1.x (Fix Safari multiple tabs by working around a Safari bug.)
@jasongrout jasongrout removed the tag:Backport label Oct 11, 2019
@lock lock bot added the status:resolved-locked label Nov 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked tag:Backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants