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

Workaround 1Password browser extension bug #1932

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jan 18, 2025

preview on auspice.us, nextstrain.org

Description of proposed changes

The bug pathologically slows down (and sometimes crashes) pages with thousands/millions of SVG elements in the DOM, which happens in reasonably sized datasets.¹

Short circuit the extension's calls to the very slow .innerText in its function:

  RM = (e) => {
    if (e == null) return null;
    let t = e.parentNode,
      n = 0;
    for (; n < 4 /*LM*/ && t !== null; ) {
      if (
        t instanceof HTMLElement &&
        t.innerText &&
        t.innerText.trim().length > 0
      )
        return t.innerText;
      (t = t.parentNode), n++;
    }
    return null;
  },

by nesting each panel's D3 elements deeper than extension's traversal limit of 4. That is, we're moving the closest HTMLElement (e.g. <div>s containing <svg>s) further away from the leaf SVG elements.

For most panels, fewer <g>s would do, but adding four guards against future changes to D3 implementation.

The workaround was trivial for entropy and frequencies panels which already have a separation where size/styles are applied on a parent <svg> and D3 operations happen on a child <g>. It was still not too bad for map and measurements panels - those did not have a pre-existing separation, but adding the separation was trivial.

The tree panel was the least trivial - the code and type annotations assumed that the root SVG element for D3 operations was an <svg>, so switching it to <g> required updating those assumptions. Additionally, there is code that retrieves the width/height from the root SVG element, so it must be retained on the child element even when nested under an <svg>, which requires width and height to be set explicitly.

¹ #1919

Related issue(s)

#1919

Checklist

  • Checks pass (docs build failure is a linting error that can be ignored)
  • If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
  • (to be done by a Nextstrain team member) Create preview PRs on downstream repositories.
  • test entropy: tb preview loads fine with 1Password extension enabled
  • test frequencies: tested locally (link TBD)
  • test map: tested locally (link TBD)
  • test tree: tested locally (link TBD)
  • test measurements: tested locally (link TBD)

The bug pathologically slows down (and sometimes crashes) pages with
thousands/millions of SVG elements in the DOM, which happens in
reasonably sized datasets.¹

Short circuit the extension's calls to the very slow .innerText in its
function:

  RM = (e) => {
    if (e == null) return null;
    let t = e.parentNode,
      n = 0;
    for (; n < 4 /*LM*/ && t !== null; ) {
      if (
        t instanceof HTMLElement &&
        t.innerText &&
        t.innerText.trim().length > 0
      )
        return t.innerText;
      (t = t.parentNode), n++;
    }
    return null;
  },

by nesting each panel's D3 elements deeper than extension's traversal
limit of 4.  That is, we're moving the closest HTMLElement (e.g. <div>s
containing <svg>s) further away from the leaf SVG elements.

For most panels, fewer <g>s would do, but adding four guards against
future changes to D3 implementation.

The workaround was trivial for entropy and frequencies panels which
already have a separation where size/styles are applied on a parent
<svg> and D3 operations happen on a child <g>. It was still not too bad
for map and measurements panels - those did not have a pre-existing
separation, but adding the separation was trivial.

The tree panel was the least trivial - the code and type annotations
assumed that the root SVG element for D3 operations was an <svg>, so
switching it to <g> required updating those assumptions. Additionally,
there is code that retrieves the width/height from the root SVG element,
so it must be retained on the child element even when nested under an
<svg>, which requires width and height to be set explicitly.

¹ <#1919>

Co-authored-by: Thomas Sibley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants