Add support for clearing selection #4720

allenjlee · 2019-03-13T07:21:25Z

This PR adds support clearing selections in Vega-Lite (#4306) for single, multi, and interval types.

In summary: there is now a clear selection transform that will clear all selected points as well as clear all panning and zooming in the visualization. The clear EventStream is mouseout for on: mouseover, and dblclick for everything else by default. Users can also specify what EventStream they want clear to be.

Here is a table of the behavior:

Interaction	Clear Behavior
Single select	Deselects Everything
Pan + Zoom w/o Data Selection	Changes back to original scale
Shift to Multi-Select	Deselects Everything
Query Widgets	Deselects everything including query sliders
Hover Line Chart	Deselects point and hides line
Brush	Deselects everything and hides brush window
Interactive Scatterplot Matrix	Deselects points and hides brush window and changes back to original scale

Files created:

clear.ts
clear.test.ts

Files modified:

selection.ts
transforms.ts
single.test.ts
toggle.test.ts
inputs.test.ts
interval.test.ts
multi.test.ts

TESTING:

clear.test.ts added (single, multi, interval tests - default, input, false).
yarn test passes
Checked against all interactive examples with local Vega editor

kanitw · 2019-03-14T01:09:29Z

Nice to see this coming along!

Some suggestions:

Once this PR is ready, it's better to squashing and merging this into a feature branch (e.g., clear) rather than master, given that we will release 3.0 soon.
It's better to have all of them ready at ship them together all at once.
Also, I know this is still a WIP, but once you request a review, you may want to make your commits a bit more atomic and make your commit message meaningful. (We want to avoid looking at git log later and see "testing editor linking".)

(Learn git commit --amend and git rebase -i if you haven't)

I hope this is helpful. I'm excited about your contribution. :)

added test suite for clear on multi adding single and interval clear adding test cases for single + interval clearing merge + squash + rebase

…ag as true at least modified default behavior and updated tests appropriately clear tx and modified dependent files testing editor linking clear tx working but slow removing unnecssary type updating tx type to modify existing signal added test suite and modified clear to not occur on default - must flag as true at least modified default behavior and updated tests appropriately readding schema file found in master branch

interval selection

adding single type support [Travis] Update schema (build: 22277) [Travis] Update examples (build: 22277) interval selection adding test cases for single + interval clearing

allenjlee · 2019-03-15T20:40:56Z

@kanitw How do I merge into a feature branch?

arvind

Hi @allenjlee, this is great work! You're very close to wrapping this up. I've left you some minor comments throughout the code. But there are also a couple of more major/general ones:

If you test with the first example (the tabular heatmap) in Add support for clearing selection #4306, you may notice that double clicking with the shift-key pressed doesn't seem to work (i.e., doesn't clear the selection any more). Double clicking w/o the shift does work. Let me know if you need help figuring out why (hint: is there logic kicking in when the shiftKey is pressed?).
Try testing this spec out which is a modified version of our interactive scatterplot matrix example. You can drag out a brush when you have the shiftKey depressed. When you double click, you'll notice that all points are colored (i.e., the selection is being cleared out correctly) but the rectangle of the brush extents persists. When we double click, we should make sure the rectangle disappears too.

{
  "$schema": "https://vega.github.io/schema/vega-lite/v3.json",
  "repeat": {
    "row": ["Horsepower", "Acceleration", "Miles_per_Gallon"],
    "column": ["Miles_per_Gallon", "Acceleration", "Horsepower"]
  },
  "spec": {
    "data": {"url": "data/cars.json"},
    "mark": "point",
    "selection": {
      "brush": {
        "type": "interval",
        "resolve": "union",
        "on": "[mousedown[event.shiftKey], window:mouseup] > window:mousemove!",
        "translate": "[mousedown[event.shiftKey], window:mouseup] > window:mousemove!",
        "zoom": "wheel![event.shiftKey]"
      }
    },
    "encoding": {
      "x": {"field": {"repeat": "column"},"type": "quantitative"},
      "y": {"field": {"repeat": "row"},"type": "quantitative"},
      "color": {
        "condition": {
          "selection": "brush",
          "field": "Origin",
          "type": "nominal"
        },
        "value": "grey"
      }
    }
  }
}

Note, the above behavior also occurs with this modified version of our interactive brush in a single scatterplot:

{
  "$schema": "https://vega.github.io/schema/vega-lite/v3.json",
  "description": "Drag out a rectangular brush to highlight points.",
  "data": {"url": "data/cars.json"},
  "selection": {
    "brush": {
        "type": "interval",
        "resolve": "union",
        "on": "[mousedown[event.shiftKey], window:mouseup] > window:mousemove!",
        "translate": "[mousedown[event.shiftKey], window:mouseup] > window:mousemove!",
        "zoom": "wheel![event.shiftKey]"
      }
  },
  "mark": "point",
  "encoding": {
    "x": {"field": "Horsepower", "type": "quantitative"},
    "y": {"field": "Miles_per_Gallon", "type": "quantitative"},
    "color": {
      "condition": {"selection": "brush", "field": "Cylinders", "type": "ordinal"},
      "value": "grey"
    }
  }
}

arvind · 2019-03-19T01:27:32Z

src/selection.ts

+   *
+   * See the [TODO: clear] documentation for more information.
+   */
+  clear?: string | boolean;


Let's set clear to be an EventStream | boolean. Even though EventStream is aliased to any, we prefer more specific typings wherever possible both as a self-documenting mechanism and for when we make the types more specific in the future.

Should EventStream be string or any?

arvind · 2019-03-19T01:29:09Z

src/selection.ts

@@ -247,13 +278,15 @@ export const defaultConfig: SelectionConfig = {
  multi: {
    on: 'click',
    fields: [SELECTION_ID],
+    clear: 'dblclick',


Not sure why multi and interval selections have a clear set by default but single doesn't? Recall that in #4306, the second example motivated the need for clearing selections using a single selection. Let's have clear: 'dblclick' on all selection types.

I had a quick question related to this that I wanted to follow up about. What should the signals array contain when I call it from the transform? I had thought it would contain all signals in the given selCmpt, but when I try to iterate through it appears some are missing. I haven't been able to figure out why this is happening.

Are transforms compiled further upstream before other parts?

arvind · 2019-03-19T01:31:14Z

src/compile/selection/transforms/clear.ts

+  signals: (model, selCmpt, signals) => {
+    const tupleIdx = signals.findIndex(i => i.name === selCmpt.name + TUPLE);
+    signals[tupleIdx].on.push({
+      events: [{source: 'scope', type: selCmpt.clear}],


Wrap selCmpt.clear with a call to parseSelector (like in translate.ts or zoom.ts). This is important to making sure that, in multi-view visualizations, clearing only fires if the user double-clicks within the same view as the selection was defined in (scope).

…orking with all example interactive visualizations

…clear off for those tests

added test suite for clear on multi adding single and interval clear adding test cases for single + interval clearing merge + squash + rebase

…ag as true at least modified default behavior and updated tests appropriately clear tx and modified dependent files testing editor linking clear tx working but slow removing unnecssary type updating tx type to modify existing signal added test suite and modified clear to not occur on default - must flag as true at least modified default behavior and updated tests appropriately readding schema file found in master branch

interval selection

adding single type support [Travis] Update schema (build: 22277) [Travis] Update examples (build: 22277) interval selection adding test cases for single + interval clearing

…orking with all example interactive visualizations

…clear off for those tests

…l/clear

kanitw · 2019-03-21T16:16:46Z

src/selection.ts

@@ -272,6 +269,7 @@ export const defaultConfig: SelectionConfig = {
  single: {
    on: 'click',
    fields: [SELECTION_ID],
+    clear: 'dblclick',


The default clear should be mouseout if on is mouseover.

(But dblclick is good otherwise. Thus, I don't think dblclick should be defined here in config.) .

Also, I don't think "interval" needs default "clear" given that click would be the default clear without an additional signal anyway. (Click triggers brush, and if you don't drag, that's equivalent to clearing.)

But dblclick is good otherwise. Thus, I don't think dblclick should be defined here in config.

Hmm, this is an interesting point. The default "on": "click" is defined in the configuration, so it seems to make sense to define clear consistently as well. But I see your point that if the user changed to "on": "mouseover" we might want to detect that and reflect it. If that's the only exception we can think of, I'd rather handle it separately, when we parse defaults, rather than not have clear defined in our SelectionConfig whatsoever.

Also, I don't think "interval" needs default "clear" given that click would be the default clear without an additional signal anyway.

I disagree. When an interval selection is bound to scales, it would be helpful to clear it to return to the original scale domains (so too if we had brush-to-zoom functionality as in #4742). Moreover, as much as possible, I would like to keep all selection types working consistently (that is, after all, our original vision of a grammar of interaction rather than a typology of selections).

Hmm, this is an interesting point. The default "on": "click" is defined in the configuration, so it seems to make sense to define clear consistently as well. But I see your point that if the user changed to "on": "mouseover" we might want to detect that and reflect it. If that's the only exception we can think of, I'd rather handle it separately, when we parse defaults, rather than not have clear defined in our SelectionConfig whatsoever.

In the rest of the language, we make the config undefined if the default value has condition rule. For example, we don't set opacity in mark config. Same for axis labelAngle.

When we want users to be able to customize config for each case, we then introduce a more specific version of config. For example, bandPaddingInner scale config is undefined (automatic) by default, but the automatic rule will draw value from barBandPaddingInner and rectBandPaddingInner scale configs. If users set bandPaddingInner scale config to a specific value, then the default value is fixed and barBandPaddingInner and rectBandPaddingInner are ignored.

So I think we should have consistent config design here.

When an interval selection is bound to scales, it would be helpful to clear it to return to the original scale domains

Good point. I agree.

Note that as I mentioned previously, I see the benefit of interaction typologies as a macro level on top of selection grammar and have started prototyping it a bit, but that's a different issue separate from this. :)

Sorry for the late reply, I just got back to the US.

Is the intention to have clear non-conditionally defined, i.e. set default configs to dblclick and in cases where user changed to "on: "mouseover" they also need to change "clear": "mouseout". Or have it conditionally defined, making the config undefined and conditionally choosing the EventStream trigger based off of the on config?

If the latter, should I make these changes in this PR or in a follow-up?

No worries at all.

I mean the latter:

making the config undefined and conditionally choosing the EventStream trigger based on the on config

Since it's a simple change, let's do in this PR?

Also, don't forget to add clear to the docs. :)

…y resets axes

…l/clear

addressed

arvind · 2019-04-05T22:21:10Z

Hi @allenjlee, nice job refactoring the code and making a first pass at the documentation! Things looked good, and I made a few updates to:

Clean up the code to follow our style guide lines (e.g., we prefer camelCase variable names rather than underscore-delimited ones).
Minimize the amount of signals that were appended. As we'd discussed, for interval selections, they only need to be added on the vname signals (e.g., brush_x or brush_y) and the rest of the logic propagates that throughout to produce the right behavior. Similarly, we only need to append to the tuple for single or multi selections.
Your table at the top of this PR correctly identifies the behavior that should occur on query widgets but I wasn't able to reproduce it. This is because the clear transform was not firing for topLevelSignals. I recall discussing this with you as well.
My first review mentioned an interaction with toggling that didn't feel quite right (double clicking with the shift key pressed). We were missing a clear trigger on the _toggle signals.
Remember to double check that any new examples you add as part of documentation are included in the PR as well. Also, to reference the examples, you need to specify them as part of the data-name HTML property (not id).

Finally, in the future, I recommend you use git rebase master to keep your branches in sync with the master branch. This strategy will avoid the merge commits polluting your history, and makes reviewing a little bit easier :)

But overall, this was an excellent first PR and I'm excited to land this functionality in the next Vega-Lite release!!

I'll hand off to @kanitw to merge, in case he has any final comments.

allenjlee · 2019-04-06T03:23:21Z

Thanks for the comments, will remember in the future!

… include explicit clear property

Adjust the size, remove unnecessary configs, make rangeSteep larger

allenjlee · 2019-04-10T09:20:05Z

Is it ok to squash and merge this in now?

kanitw · 2019-04-10T15:40:28Z

Yeah, let's do it. I found some problem when I tried to replicate the stock example, but it might be a different problem somewhere else. Given this is working well for basic functionality already, let's get this in.

kanitw · 2019-04-10T15:41:13Z

@allenjlee Congrats on your PR. 🎉

ps. I'll create the example and file an issue later.

domoritz · 2019-04-11T06:13:40Z

🎉 Congratulations on this PR @allenjlee. This is a great improvement.

allenjlee requested a review from arvind March 13, 2019 07:21

arvind added the WIP 🚧 label Mar 13, 2019

Allen Lee added 4 commits March 15, 2019 16:21

added clear transform to files

7989028

added test suite for clear on multi adding single and interval clear adding test cases for single + interval clearing merge + squash + rebase

adding single type support

5821af8

interval selection

adding test cases for single + interval clearing

ef5f6b8

adding single type support [Travis] Update schema (build: 22277) [Travis] Update examples (build: 22277) interval selection adding test cases for single + interval clearing

allenjlee force-pushed the ajl/clear branch from 5b5ccfe to ef5f6b8 Compare March 15, 2019 20:28

allenjlee changed the title ~~Add support for clearing selection - MultiSelection~~ Add support for clearing selection Mar 15, 2019

[Travis] Update examples (build: 22361)

dd06819

arvind reviewed Mar 19, 2019

View reviewed changes

Allen Lee and others added 12 commits March 21, 2019 00:53

Clear transform default doubleclick clears selected data and brush, w…

de29ed0

…orking with all example interactive visualizations

adding test cases for clear.ts and updating other test files to turn …

0c3d122

…clear off for those tests

added clear transform to files

1505a1e

added test suite for clear on multi adding single and interval clear adding test cases for single + interval clearing merge + squash + rebase

adding single type support

3cba482

interval selection

adding test cases for single + interval clearing

d49d694

adding single type support [Travis] Update schema (build: 22277) [Travis] Update examples (build: 22277) interval selection adding test cases for single + interval clearing

[Travis] Update examples (build: 22361)

20eba3f

Clear transform default doubleclick clears selected data and brush, w…

cd6362a

…orking with all example interactive visualizations

adding test cases for clear.ts and updating other test files to turn …

94027a9

…clear off for those tests

Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…

f1f08d7

…l/clear

[Travis] Update schema (build: 22402)

cb15c08

[Travis] Update examples (build: 22402)

2e8b2b5

kanitw reviewed Mar 21, 2019

View reviewed changes

Allen Lee and others added 5 commits April 2, 2019 03:20

migrated clear to conditional config, and interval type clear now onl…

c9ab937

…y resets axes

Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…

70b4ea3

…l/clear

adding clear transform documentation

e6335a1

Merge branch 'master' into ajl/clear

d505802

[Travis] Update schema (build: 22802)

30857fe

Allen Lee and others added 2 commits April 3, 2019 02:21

clearing selection and panning/zooming, also updated docs and tests

ae4f8ae

[Travis] Update examples (build: 22820)

3cdba25

kanitw mentioned this pull request Apr 3, 2019

Revise / Clarify if there are differences between selection properties and selection transforms #4841

Closed

Allen Lee added 3 commits April 5, 2019 08:06

significantly refactored clear code

f0804b2

Merge branch 'ajl/clear' of https://github.com/vega/vega-lite into aj…

77aa146

…l/clear

updating TSDocs and Vega-lite docs

d105409

allenjlee requested a review from kanitw April 5, 2019 12:19

[Travis] Update examples (build: 22858)

937aa9f

arvind added 3 commits April 5, 2019 17:12

Minor fixes to clear logic/style to minimize signal additions.

1a3ec0c

Append clear to top-level signals to account for input bindings.

053f24b

Refine documentation on selection clearing.

519a2b6

arvind removed the WIP 🚧 label Apr 5, 2019

Selection clearing should account for toggling.

6b81dd2

arvind force-pushed the ajl/clear branch from 3809038 to 6b81dd2 Compare April 5, 2019 22:31

Travis CI added 2 commits April 5, 2019 22:38

[Travis] Update schema (build: 22868)

b99ef92

[Travis] Update examples (build: 22868)

f6e9a88

kanitw and others added 4 commits April 5, 2019 20:48

Fix backticks

9455fb8

Rename selection_clear_heatmap to selection_heatmap as it doesn't…

e51e6ce

… include explicit clear property

Refactor selection_heatmap example + add it to the gallery

165d8ca

Adjust the size, remove unnecessary configs, make rangeSteep larger

[Travis] Update examples (build: 22876)

dd9d82e

kanitw merged commit 7d44f30 into master Apr 10, 2019

kanitw deleted the ajl/clear branch April 10, 2019 15:40

jheer mentioned this pull request Apr 15, 2019

VL 3.2 breaks multi mouseover selection handling? #4879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for clearing selection #4720

Add support for clearing selection #4720

allenjlee commented Mar 13, 2019 •

edited

Loading

kanitw commented Mar 14, 2019

allenjlee commented Mar 15, 2019

arvind left a comment

arvind Mar 19, 2019

domoritz Mar 19, 2019

arvind Mar 19, 2019

allenjlee Mar 19, 2019

arvind Mar 19, 2019

kanitw Mar 21, 2019

kanitw Mar 21, 2019

arvind Mar 22, 2019 •

edited

Loading

kanitw Mar 22, 2019

allenjlee Apr 1, 2019

kanitw Apr 1, 2019 •

edited

Loading

kanitw Apr 1, 2019

arvind commented Apr 5, 2019 •

edited

Loading

allenjlee commented Apr 6, 2019

allenjlee commented Apr 10, 2019

kanitw commented Apr 10, 2019

kanitw commented Apr 10, 2019

domoritz commented Apr 11, 2019

Add support for clearing selection #4720

Add support for clearing selection #4720

Conversation

allenjlee commented Mar 13, 2019 • edited Loading

kanitw commented Mar 14, 2019

allenjlee commented Mar 15, 2019

arvind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvind Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvind commented Apr 5, 2019 • edited Loading

allenjlee commented Apr 6, 2019

allenjlee commented Apr 10, 2019

kanitw commented Apr 10, 2019

kanitw commented Apr 10, 2019

domoritz commented Apr 11, 2019

allenjlee commented Mar 13, 2019 •

edited

Loading

arvind Mar 22, 2019 •

edited

Loading

kanitw Apr 1, 2019 •

edited

Loading

arvind commented Apr 5, 2019 •

edited

Loading