Highlight moves when hovering over the copy/delete buttons

[johndoknjas]

Closes #17246.

Btw, this PR contains the merged commits for #17261. So, this PR should be merged after (or in place of) that one.

[ornicar]

these elements are managed by snabbdom. Don't interfer with direct dom mutations.

[schlawg]

if you wanted to go all generic and future proof, you could add something like below to analyse/src/treeView/treeView.ts

example implementation:

// the from/to arguments for add/remove are absolute, where from="" is root
// and to="*" selects all descendants of from

export class PathClassMap {
  private map: Map<Tree.Path, Map<Tree.Path, Set<string>>> = new Map();

  add(className: string, from: Tree.Path, to: Tree.Path): void {
    const fromMap = this.map.get(from) ?? new Map();
    const toSet = fromMap.get(to) ?? new Set();
    toSet.add(className);
    fromMap.set(to, toSet);
    this.map.set(from, fromMap);
  }

  remove(className: string, from: Tree.Path, to: Tree.Path): void {
    const fromMap = this.map.get(from);
    if (!fromMap) return;
    const toSet = fromMap.get(to);
    if (!toSet) return;
    toSet.delete(className);
    if (toSet.size === 0) fromMap.delete(to);
    if (fromMap.size === 0) this.map.delete(from);
  }

  get(at: Tree.Path): string[] {
    const classes: string[] = [];
    for (const [from, toMap] of this.map.entries()) {
      if (!at.startsWith(from)) continue;
      for (const [to, classSet] of toMap.entries()) {
        if (to === '*' || at.startsWith(to)) classes.push(...classSet);
      }
    }
    return classes;
  }
}

instantiate a new class member pathClasses: PathClassMap in AnalyseCtrl and call ctrl.pathClasses.add/remove then redraw from your mouseenter/mouseleave handlers.

ctrl.pathClasses.get() would be added to the vdom in analyse/src/treeView/common.ts nodeClasses function.

it's 40 lines of new boilerplate but maybe reusable.

[ornicar]

not sure that's the simplest way to do it. Can't we just

h('node', {
  class: {
    highlight: highlightPath.startsWith(nodePath)
  }
}, /* ... */)

[schlawg]

is it the simplest way to do { class: { highlightPath.startsWith(nodePath) } }, no it is not. but it is extensible wrt class tagging and supports "select from root to here" which you need for copy pgn hiliting. it could be used in the future to provide other bells and whistles on variation nodes. maybe that's the best argument against it.

[ornicar]

Yeah I just don't want to start messing with the DOM that snabbdom manages. Transient or not, It's a recipe for disaster.

"select from root to here" can also be implemented into {class: {highlight: isHighlighted(nodePath)} }

[johndoknjas]

@ornicar @schlawg Any consensus on which method we're going with? :)

[schlawg]

That could work altho I'd prefer staying stateless if possible, to eliminate the need to clean things up, and the consequences for not doing it perfectly. Statelessness would be more computationally expensive but as long as it's only while playing with the context menu, that would be fine.

stateless how? redraw() has no arguments so it seems to me that mouseenter/mouseleave must either set the DOM classes directly or set ctrl state so that nodeClasses knows what to do on redraw.

[ornicar]

stateless as in we don't manage Map<Tree.Path, Map<Tree.Path, Set<string>>>. We have a mutable Tree.Path in the ctrl, and then the node classes are computed based on it during rendering.

[schlawg]

ok, but you'll need at least two new values in ctrl to support highlighting both "delete from here" and "copy as pgn" because they have different tree behaviors.

@johndoknjas I think the consensus is ignore the helper PathClassMap I suggested in treeView.ts and do what thibault recommends, i.e. put whatever you need in ctrl.

[schlawg]

to be fair, we still compute the node classes during rendering with the map implementation

  get(at: Tree.Path): string[] {
    const classes: string[] = [];
    for (const [from, toMap] of this.map.entries()) {
      if (!at.startsWith(from)) continue;
      for (const [to, classSet] of toMap.entries()) {
        if (to === '*' || at.startsWith(to)) classes.push(...classSet);
      }
    }
    return classes;
  }

that's why i didn't understand stateless. i think you meant mapless.

[ornicar]

that doesn't seem to be working as intended

[johndoknjas]

@ornicar Thanks for finding that bug, I've fixed it. The issue was that backslashes needed to be escaped again before being given to querySelector.

these elements are managed by snabbdom. Don't interfer with direct dom mutations.

For this though, I'm not sure how else to update the classlist.

[schlawg]

@ornicar Thanks for finding that bug, I've fixed it. The issue was that backslashes needed to be escaped again before being given to querySelector.

these elements are managed by snabbdom. Don't interfer with direct dom mutations.

For this though, I'm not sure how else to update the classlist.

you could maintain a "hoverPath" and a "hoverAction" variable in a nice dumping ground like AnalyseCtrl, set and unset them in mouseenter/mouseleave handlers on the context menu items, fire a redraw, and manage the styles when building the snab tree.

i'm more concerned that when hovering "delete from here", the context menu does a terrific job of hiding almost all of the hilited nodes. it's not the best.

[schlawg]

that said, i'm not sure it's preferable to toggle the classes in a snab-compliant way. they're cosmetic and transient. the way you do it is less code and i don't see it causing any problems except when external modifications arrive during a hover, which a reload would fix.

[schlawg]

hey, let's not let this slip through the cracks. john, do you have any ideas for the fact that context menu covers the nodes being deleted? i was thinking the menu itself could fade into transparency to reveal the nodes behind when hovering delete only.

what do you think about that?

[johndoknjas]

@schlawg That's a good idea, I think it would work well.

[schlawg]

we'll see. there's not really precedent for the thing you're interacting with going slightly transparent on websites so i could imagine it feeling rather weird.

[johndoknjas]

@schlawg Updated to make it transparent.