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

[vega] support HTML tooltips #17632

Merged
merged 12 commits into from Apr 27, 2018

Conversation

Projects
None yet
7 participants
@nyurik
Copy link
Contributor

commented Apr 10, 2018

Implement support for the richer style Vega tooltips, per #17215 request. Uses Vega tooltip plugin for formatting. Always enabled unless user sets tooltips=false flag in the config.kibana section of the spec.
image

Test code

{
  $schema: https://vega.github.io/schema/vega/v3.json
  config: {
    kibana: {
      tooltips: {
        // always center on the mark, not mouse x,y
        centerOnMark: true
        position: top
        padding: 20
      }
    }
  }
  data: [
    {
      name: table
      values: [
        {
          title: This is a long title
          fieldA: value of fld1
          fld2: 42
        }
      ]
    }
  ]
  marks: [
    {
      from: {data: "table"}
      type: rect
      encode: {
        enter: {
          fill: {value: "#060"}
          x: {signal: "40"}
          y: {signal: "40"}
          width: {signal: "40"}
          height: {signal: "40"}
          tooltip: {signal: "datum || null"}
        }
      }
    }
  ]
}
@elasticmachine

This comment has been minimized.

Copy link

commented Apr 10, 2018

@nyurik nyurik requested review from timroes and kimjoar Apr 10, 2018

@nyurik nyurik force-pushed the nyurik:vegatooltip branch from b9e596c to 2dca0d2 Apr 19, 2018

@nyurik nyurik changed the title [WIP] support automatic vega tooltips support automatic vega tooltips Apr 19, 2018

@nyurik nyurik requested a review from thomasneirynck Apr 19, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 19, 2018

@nyurik nyurik requested a review from ppisljar Apr 19, 2018

@ppisljar
Copy link
Member

left a comment

if i understand correctly, we are adding this as the default tooltip handling only shows a single value ?

Adapted from https://github.com/vega/vega-tooltip/blob/master/src/vega-tooltip.css
*/

.vg-tooltip {

This comment has been minimized.

Copy link
@ppisljar

ppisljar Apr 23, 2018

Member

should match https://elastic.github.io/eui/#/display/tooltip (or even better, but not sure if possible, use it)

@nyurik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

vega tooltip has been significantly reworked over the weekend, update coming soon.

@nyurik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2018

The new support will work from the tooltip channel in Vega itself, capable of showing a table or a single value.

@ppisljar
Copy link
Member

left a comment

i dont think this completely matches the EUI styles ?

@nyurik nyurik force-pushed the nyurik:vegatooltip branch from 8881fa4 to baa9e7d Apr 24, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 24, 2018

el.setAttribute('id', tooltipId);
el.classList.add('euiToolTipPopover', 'euiToolTip', `euiToolTip--${this.position}`);
if (createElement) {
document.querySelector('body').appendChild(el);

This comment has been minimized.

Copy link
@domoritz

domoritz Apr 24, 2018

document.body

nyurik added some commits Apr 10, 2018

@nyurik nyurik force-pushed the nyurik:vegatooltip branch from baa9e7d to 4725940 Apr 26, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 27, 2018

@nyurik nyurik changed the title support automatic vega tooltips [vega] support HTML tooltips Apr 27, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 27, 2018

nyurik added some commits Apr 27, 2018

@nyurik nyurik force-pushed the nyurik:vegatooltip branch from 78a8233 to c81fff3 Apr 27, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 27, 2018

// Sanitized HTML is created by the tooltip library,
// with a largue nmuber of tests, hence supressing eslint here.
// eslint-disable-next-line no-unsanitized/property
el.innerHTML = createTooltipContent(value, escapeHTML);

This comment has been minimized.

Copy link
@timroes

timroes Apr 27, 2018

Contributor

@elastic/kibana-security Could we have a validation from you, that vega-tooltip is safe enough to use for applying it's results to innerHTML?

This comment has been minimized.

Copy link
@kobelb

kobelb Apr 27, 2018

Contributor

Given the context of where the escaped HTML is being input, vega-tooltip's escapeHTML function appears to be "safe enough". However, it's only escaping < and & so if vega-tooltip began using value to set attributes on HTML elements, we would be vulnerable to an XSS. We're using _.escape elsewhere to escape HTML, so i'd prefer we use _.escape instead of vega-tooltip.escapeHTML to further protect ourselves.

This comment has been minimized.

Copy link
@nyurik

nyurik Apr 27, 2018

Author Contributor

@kobelb thx, done.

@ppisljar
Copy link
Member

left a comment

LGTM

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 27, 2018

@kobelb

kobelb approved these changes Apr 27, 2018

Copy link
Contributor

left a comment

LGTM - from the security perspective.

@thomasneirynck
Copy link
Contributor

left a comment

only gave this a cursory look, but lgtm from me. verified final commit addresses security issue.


const result = this._config.tooltips || {};

if (result.position === undefined) {

This comment has been minimized.

Copy link
@thomasneirynck

thomasneirynck Apr 27, 2018

Contributor

no need to change, but in future I'd use typeof .... === "undefined", as it's more canonical and doesn't rely on obscure globals.

@nyurik nyurik merged commit b300818 into elastic:master Apr 27, 2018

1 check passed

kibana-ci Build finished.
Details

@nyurik nyurik deleted the nyurik:vegatooltip branch Apr 27, 2018

nyurik added a commit to nyurik/kibana that referenced this pull request Apr 27, 2018

[vega] support HTML tooltips (elastic#17632)
Implement support for the richer style Vega tooltips, per elastic#17215 request. Uses [Vega tooltip plugin](https://github.com/vega/vega-tooltip) for formatting.  Always enabled unless user sets `tooltips=false` flag in the `config.kibana` section of the spec.
![image](https://user-images.githubusercontent.com/1641515/39344487-7abc9eb0-49eb-11e8-89dd-bf562563ae1c.png)

## Test code
```js
{
  $schema: https://vega.github.io/schema/vega/v3.json
  config: {
    kibana: {
      tooltips: {
        // always center on the mark, not mouse x,y
        centerOnMark: true
        position: top
        padding: 20
      }
    }
  }
  data: [
    {
      name: table
      values: [
        {
          title: This is a long title
          fieldA: value of fld1
          fld2: 42
        }
      ]
    }
  ]
  marks: [
    {
      from: {data: "table"}
      type: rect
      encode: {
        enter: {
          fill: {value: "elastic#60"}
          x: {signal: "40"}
          y: {signal: "40"}
          width: {signal: "40"}
          height: {signal: "40"}
          tooltip: {signal: "datum || null"}
        }
      }
    }
  ]
}
```

@nyurik nyurik added v6.4.0 and removed v6.3.0 labels Apr 27, 2018

nyurik added a commit that referenced this pull request Apr 27, 2018

[vega] support HTML tooltips (#17632) (#18642)
Implement support for the richer style Vega tooltips, per #17215 request. Uses [Vega tooltip plugin](https://github.com/vega/vega-tooltip) for formatting.  Always enabled unless user sets `tooltips=false` flag in the `config.kibana` section of the spec.
![image](https://user-images.githubusercontent.com/1641515/39344487-7abc9eb0-49eb-11e8-89dd-bf562563ae1c.png)

## Test code
```js
{
  $schema: https://vega.github.io/schema/vega/v3.json
  config: {
    kibana: {
      tooltips: {
        // always center on the mark, not mouse x,y
        centerOnMark: true
        position: top
        padding: 20
      }
    }
  }
  data: [
    {
      name: table
      values: [
        {
          title: This is a long title
          fieldA: value of fld1
          fld2: 42
        }
      ]
    }
  ]
  marks: [
    {
      from: {data: "table"}
      type: rect
      encode: {
        enter: {
          fill: {value: "#60"}
          x: {signal: "40"}
          y: {signal: "40"}
          width: {signal: "40"}
          height: {signal: "40"}
          tooltip: {signal: "datum || null"}
        }
      }
    }
  ]
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.