When "focus selected", exiting single click-focused sequence with Esc keeps focus on single sequence

[corneliusroemer]

When the "Focus on selected" toggle is ON, and one clicks on a single sequence to focus it and then hits the Escape key to defocus the sequence, the modal disappears but the focus remains on the single (previously focused) sequence in disagreement with actual filters

How to reproduce

1. Toggle on "Focus on selected"
2. Click on a single sequence/tip with mouse
3. Hit Escape key
4. Observe results

Current behavior

Focus is as if one had filtered for the sequence one had clicked before

Expected behavior

Focus is back to how it was before clicking on the single sequence/tip

Your environment: if running Nextstrain locally

• Operating system: macOS
• Browser: Chrome
• Version (e.g. auspice 2.7.0): Auspice v2.66.0

[victorlin]

Interesting, this seems to be a timing issue. It doesn't happen for me when clicking outside of the modal to dismiss it, unless I add a delay here:

--- a/src/components/tree/phyloTree/change.ts
+++ b/src/components/tree/phyloTree/change.ts
@@ -438,7 +438,9 @@ export const change = function change(
 
   /** display order calculations */
   if (newDistance || updateLayout || changeNodeOrder || streamDefinitionChange) {
+    setTimeout(() => {
     setDisplayOrder({nodes: this.nodes, focus, streams: this.params.showStreamTrees && this.streams});
+    }, 100);
   } else if (this.params.showStreamTrees && (changeColorBy || changeVisibility)) {
     // rippleDisplayOrders are typically called by setDisplayOrder however for ∆{colorBy,visibility} we don't want to pay
     // the price of recomputing the display orders for the entire tree, we just need to recompute the

[victorlin]

This isn't specific to focus; it also affects the logic that renders tip labels. It's less reproducible than the focus bug, but happens reliably if you click then hit the Escape key right away.

bug.mov

[victorlin]

A minimal patch addressing the focus bug:

--- a/src/components/tree/phyloTree/change.ts
+++ b/src/components/tree/phyloTree/change.ts
@@ -437,8 +437,10 @@ export const change = function change(
    */
 
   /** display order calculations */
-  if (newDistance || updateLayout || changeNodeOrder || streamDefinitionChange) {
+  if (newDistance || updateLayout || changeNodeOrder || streamDefinitionChange || changeVisibility) {
+    queueMicrotask(() => {
     setDisplayOrder({nodes: this.nodes, focus, streams: this.params.showStreamTrees && this.streams});
+    });
   } else if (this.params.showStreamTrees && (changeColorBy || changeVisibility)) {
     // rippleDisplayOrders are typically called by setDisplayOrder however for ∆{colorBy,visibility} we don't want to pay
     // the price of recomputing the display orders for the entire tree, we just need to recompute the

[victorlin]

and for the tip label bug:

--- a/src/components/tree/phyloTree/change.ts
+++ b/src/components/tree/phyloTree/change.ts
@@ -177,7 +177,9 @@ export const modifySVG = function modifySVG(
   if (extras.hideTipLabels) {
     this.removeTipLabels();
   } else if (elemsToUpdate.has('.tipLabel')) {
+    queueMicrotask(() => {
     this.updateTipLabels();
+    });
   }
   if (elemsToUpdate.has('.grid')) {
     if (this.grid && this.layout !== "unrooted") this.addGrid();

[jameshadfield]

I'm extremely wary of messing with order-of-execution (such as queueMicrotask) but equally it'd be nice to avoid bugs like this. My guess is that it's back-to-back redux actions causing these bugs, but I've not investigated this particular case.

P.S. setDisplayOrder isn't cheap, and so I'd like to avoid running it on changeVisibility, especially w.r.t. animation. The complexity of change.js is due to us trying to calculate the minimal work needed each update.

[victorlin]

Yeah, I'm not convinced it's the only way to solve the bug. There's probably something else, but haven't found it yet.

setDisplayOrder isn't cheap, but isn't it effectively running on most visibility changes already? I thought that's what allows this to work:

focus.mov

[jameshadfield]

setDisplayOrder isn't cheap, but isn't it effectively running on most visibility changes already? I thought that's what allows this to work:

Oh yeah, it must run now that we have focus. But it shouldn't be (as in, conceptually it shouldn't be) if we've not toggle on "focus on selected". Likewise, it shouldn't run when we're clicking on tips and focus isn't toggled on.

[corneliusroemer]

Slight aside on Victor's comment re reproducibility: I'm on an M4 Max and due to the machine's speed, I encounter some bugs that others on slower hardware don't see. E.g. in Loculus, when I run Playwright tests, things fail on my machine but are fine on CI (which has only 2-4 slower cores) or other dev laptops.