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

Experiment: add support for distinguishing hover & click events #1979

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

yurivish
Copy link
Contributor

@yurivish yurivish commented Jan 30, 2024

This is a tiny experimental patch that addresses #1832 by sending Boolean "sticky" information via the event's detail option.

Plot's behavior is otherwise unchanged. Since the sticky data is part of the event, a user can now create a generator cell that responds to the "substream" of click-relevant events. Example:

click = Generators.observe((notify) => {
  let lastValue = null;
  const listener = (e) => {
    // If e.detail is true, then this was a click (sticky) event.
    // If e.detail is false, then (this feels untidy) deselect if the value is null.
    if (e.detail || (viewof hover.value === null && lastValue !== null)) {
      lastValue = viewof hover.value;
      notify(lastValue);
    }
  };
  viewof hover.addEventListener("input", listener);
  notify(lastValue);
  return () => viewof hover.removeEventListener("input", listener);
})

There might be unfavorable trade-offs to the idea of using the detail option for this, and the null handling is somewhat untidy (though maybe there's a better way?) so please feel free to close if it isn't a good idea. But I was surprised and pleased with the fact that this was such a small code change.

Edit: lol, it fails tests. Sorry! I forgot to check that... Good thing I labeled this an experiment. The issue is that the event is now a CustomEvent, since that's the type that supports adding detail.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I’m cool with this conceptually, but I’m wondering if we can

  • create a custom subclass of Event instead of using CustomEvent
  • expose the sticky state as event.sticky instead of event.detail

If a subclass isn’t possible, then I would prefer detail to be an object {sticky} instead of the boolean for better future compat.

@yurivish
Copy link
Contributor Author

yurivish commented Jan 31, 2024

I changed it to use an Event subclass, but jsdom is still unhappy. There are lots of errors like this:

 1) plot autoArea:
     TypeError: Failed to execute 'dispatchEvent' on 'EventTarget': parameter 1 is not of type 'Event'.
      at exports.convert (node_modules/jsdom/lib/jsdom/living/generated/Event.js:22:9)
      at SVGSVGElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:236:24)
      at context.dispatchValue (file:///home/runner/work/plot/plot/src/plot.js:179:12)
      at render (file:///home/runner/work/plot/plot/src/interactions/pointer.js:129:59)
      at Tip.<anonymous> (file:///home/runner/work/plot/plot/src/interactions/pointer.js:182:14)
      at Tip.render (file:///home/runner/work/plot/plot/src/mark.js:144:15)
      at plot (file:///home/runner/work/plot/plot/src/plot.js:288:25)
      at Array.plotThis [as plot] (file:///home/runner/work/plot/plot/src/plot.js:372:10)
      at autoArea (test/plots/autoplot.ts:58:65)
      at async doesNotWarnAsync (file:///home/runner/work/plot/plot/test/assert.js:53:14)
      at async file:///home/runner/work/plot/plot/test/plot.js:12:18
      at async Context.<anonymous> (file:///home/runner/work/plot/plot/test/jsdom.js:29:14)

The error is thrown from a generated file that is not in source control, but here are the relevant lines from my local copy.

I am not familiar with jsdom, but some searching found a similar issue that was mitigated with a "fix" that looks similar to what Plot already does:

I can "fix" it by setting global.Event = window.Event, but realize from this thread that this is discouraged.

which sounds similar to (or the same as) what Plot already does:

global.Event = jsdom.window.Event;

I don't quite understand what is happening yet. Will have another look tomorrow.

…, but alias the global jsdom EventTarget to the window EventTarget...
…op level, but alias the global jsdom EventTarget to the window EventTarget..."

This reverts commit 73a0e67.
@yurivish
Copy link
Contributor Author

yurivish commented Jan 31, 2024

Moving the event definition inside the plot function resolves the jsdom issue.

I thought it might help since by the time the function runs it's definitely inside the right global context, but I wonder why that wasn't the case outside the function.

@Fil
Copy link
Contributor

Fil commented Jan 31, 2024

I wonder why that wasn't the case outside the function.

As I understand it it's because it happens earlier and thus the class extends whatever is the global.Event at that point.

The addition of the custom _sticky property to the figure element exposes this value; what we want is to cache it so we can compare it on the next event. So I wonder if we could store this state in a variable named stickyState and created parallel to context?, or add it to the context instead?

Please add a representative use case in test/plots/ so we can test these changes more easily. (See https://github.com/observablehq/plot/blob/main/test/plots/pointer.ts#L28 for an example.)

@yurivish
Copy link
Contributor Author

yurivish commented Feb 5, 2024

I'm not sure if it's due to this issue or something else but the test command fails to regenerate output files if I delete them as per the instructions for regenerating the snapshot tests. This is on an M2 mac on Sonoma 14.2.1 with node v21.5.0.

I tried generating an initial snapshot manually, but this gets rejected due to differences between the true output and the manual version. Comparing the HTML I copied from a notebook and the HTML in the pointerViewof output test, there seem to be lots of small differences so it doesn't seem to trivial to make a correct snapshot manually. Maybe someone else can generate the initial snapshot?

Let me know if the general idea of the test is reasonable – it dynamically shows the click selection in a textarea similar to the way the viewof test shows the value.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

I'll take a pass at generating the test

src/plot.js Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor

Fil commented Feb 5, 2024

To generate the test output I did:

nvm use 20
yarn
rm test/output/pointerSticky.html
yarn test

I also had to run

yarn prettier -w test/plots/pointer.ts

And finally

git add test/output/pointerSticky.html test/plots/pointer.ts

I don't think I can push to your branch, though.

@yurivish
Copy link
Contributor Author

yurivish commented Feb 6, 2024

Thank you! I did not know about nvm. Tests passed locally on node 20.

@Fil
Copy link
Contributor

Fil commented Feb 6, 2024

In the brush PR #1653 I'm following a slightly different pattern (with the same goal of telling the listener whether we're "browsing" or "selecting"). This PR attaches a .done property to the value on brush end.

Pros and cons:

  • value.done is simpler to read; 👍
  • value.done is consistent with what Observable does when streaming sql; 👍
  • done as a concept is more generic than sticky; 👍
  • value.done messes with the value, and cannot be applied generically on any value (it needs to be an array, and we should not mutate the original array of data when the selection is "full"). 👎

Whatever route we take we should try to be as generic as possible (and as consistent as what makes sense).

@yurivish
Copy link
Contributor Author

yurivish commented Feb 6, 2024

Another aspect of generality is whether you anticipate ever wanting to support simultaneous hover / click selections. With the current event stream we assume that once a new non-sticky event comes in, the selected datum has been deselected. I've found the ability to support hovers and clicks at the same time to be very useful in more app-like contexts, where they can be used to show comparisons between the selected element and other (hovered) elements.

@yurivish
Copy link
Contributor Author

yurivish commented Feb 6, 2024

I think that the information about the kind of selection should be communicated out-of-band, ie. not on the datum itself. I think this means that it would either be a property of the event object, or a wrapper object that contains the datum along with any metadata, eg. plot.value = { done: ..., value: ... }. There might be other options too.

To me, the word done seems to carry an implication that each hover selection is just an unfinished click selection, but I'll leave it to you and Mike to pick the best fit for the idea; sticky is also a bit odd to me too, though both make sense :)

There's also a third type of interaction one can get with some styluses, which is a true "hover" interaction where the cursor does not even touch the tablet: https://astropad.com/blog/apple-pencil-hover-everything-you-need-to-know/. I think there's a way to tell these apart from "touch" pointer events using the PointerEvents API but I'm not sure.

@Fil
Copy link
Contributor

Fil commented Feb 6, 2024

We won't change plot.value, but it's worth thinking about exposing this alongside the value; I agree that done is a very limiting word. The example use case (click to fix 1 point & hover to compare) is very compelling. Reminds me of the sleepwalk UI to read distances on a (UMAP-learn) map: https://anders-biostat.github.io/sleepwalk/ & https://observablehq.com/@fil/umap-sleepwalk

src/plot.js Outdated Show resolved Hide resolved
@yurivish
Copy link
Contributor Author

I noticed that on my iPhone, the example notebook only ever sets the hover variable, and not click. It's not immediately obvious to me why that might be – any ideas?

@yurivish
Copy link
Contributor Author

yurivish commented Jul 29, 2024

Re: CI failure: looks like there was a reason we moved the event inside the plot function...

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.

3 participants