From 6f26baab98aa59d63b0c51334c37736665e592f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 4 May 2016 14:17:38 -0400 Subject: [PATCH 1/6] use Drawing.setClipUrl when creating subplots and range slider nodes: - So that clip paths work in cases where is set, for example in some version of AngularJS https://github.com/angular/angular.js/issues/8934 --- src/components/rangeslider/range_plot.js | 4 +++- src/plot_api/plot_api.js | 6 ++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/rangeslider/range_plot.js b/src/components/rangeslider/range_plot.js index 39dc6e1aa5d..b70f5dbab5f 100644 --- a/src/components/rangeslider/range_plot.js +++ b/src/components/rangeslider/range_plot.js @@ -8,6 +8,8 @@ 'use strict'; +var d3 = require('d3'); + var Symbols = require('../drawing/symbol_defs'); var Drawing = require('../drawing'); @@ -38,7 +40,7 @@ module.exports = function rangePlot(gd, w, h) { clipDefs.appendChild(clip); var rangePlot = document.createElementNS(svgNS, 'g'); - rangePlot.setAttribute('clip-path', 'url(#range-clip-path)'); + d3.select(rangePlot).call(Drawing.setClipUrl, 'range-clip-path'); rangePlot.appendChild(clipDefs); diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index dc6940341e0..6ed76ab6270 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2913,10 +2913,8 @@ function lsInner(gd) { }); - plotinfo.plot.attr({ - 'transform': 'translate(' + xa._offset + ', ' + ya._offset + ')', - 'clip-path': 'url(#' + clipId + ')' - }); + plotinfo.plot.call(Lib.setTranslate, xa._offset, ya._offset); + plotinfo.plot.call(Drawing.setClipUrl, clipId); var xlw = Drawing.crispRound(gd, xa.linewidth, 1), ylw = Drawing.crispRound(gd, ya.linewidth, 1), From c1f4598c13aca57c7d3e55d110637b0ca3d748fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 4 May 2016 14:17:56 -0400 Subject: [PATCH 2/6] add test for Drawing.setClipUrl --- test/jasmine/tests/drawing_test.js | 45 ++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 test/jasmine/tests/drawing_test.js diff --git a/test/jasmine/tests/drawing_test.js b/test/jasmine/tests/drawing_test.js new file mode 100644 index 00000000000..9bb648715f9 --- /dev/null +++ b/test/jasmine/tests/drawing_test.js @@ -0,0 +1,45 @@ +var Drawing = require('@src/components/drawing'); + +var d3 = require('d3'); + + +describe('Drawing.setClipUrl', function() { + 'use strict'; + + beforeEach(function() { + this.svg = d3.select('body').append('svg'); + this.g = this.svg.append('g'); + }); + + it('should set the clip-path attribute', function() { + expect(this.g.attr('clip-path')).toBe(null); + + Drawing.setClipUrl(this.g, 'id1'); + + expect(this.g.attr('clip-path')).toEqual('url(#id1)'); + }); + + it('should unset the clip-path if arg is falsy', function() { + this.g.attr('clip-path', 'url(#id2)'); + + Drawing.setClipUrl(this.g, false); + + expect(this.g.attr('clip-path')).toBe(null); + }); + + it('should append window URL to clip-path if is present', function() { + + // append with href + d3.select('body') + .append('base') + .attr('href', 'https://plot.ly'); + + // grab window URL + var href = window.location.href; + + Drawing.setClipUrl(this.g, 'id3'); + + expect(this.g.attr('clip-path')) + .toEqual('url(' + href + '#id3)'); + }); +}); From 34829f301c71dd7e1166802f30e39103bfd55cda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 4 May 2016 14:18:36 -0400 Subject: [PATCH 3/6] add test (all clip paths are compatible in envs) --- test/jasmine/tests/cartesian_test.js | 70 ++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/test/jasmine/tests/cartesian_test.js b/test/jasmine/tests/cartesian_test.js index 38c09608cef..0893598a3ff 100644 --- a/test/jasmine/tests/cartesian_test.js +++ b/test/jasmine/tests/cartesian_test.js @@ -49,3 +49,73 @@ describe('zoom box element', function() { .toEqual(0); }); }); + +describe('plot svg clip paths', function() { + + // plot with all features that rely on clip paths + function plot() { + return Plotly.plot(createGraphDiv(), [{ + type: 'contour', + z: [[1,2,3], [2,3,1]] + }, { + type: 'scatter', + y: [2, 1, 2] + }], { + showlegend: true, + xaxis: { + rangeslider: {} + }, + shapes: [{ + xref: 'x', + yref: 'y', + x0: 0, + y0: 0, + x1: 3, + y1: 3 + }] + }); + } + + afterEach(destroyGraphDiv); + + it('should set clip path url to ids (base case)', function(done) { + plot().then(function() { + + d3.selectAll('[clip-path]').each(function() { + var cp = d3.select(this).attr('clip-path'); + + expect(cp.substring(0, 5)).toEqual('url(#'); + expect(cp.substring(cp.length-1)).toEqual(')'); + }); + + done(); + }); + }); + + it('should set clip path url to ids appended to window url', function(done) { + + // this case occurs in some past versions of AngularJS + // https://github.com/angular/angular.js/issues/8934 + + // append with href + d3.select('body') + .append('base') + .attr('href', 'https://plot.ly'); + + // grab window URL + var href = window.location.href; + + plot().then(function() { + + d3.selectAll('[clip-path]').each(function() { + var cp = d3.select(this).attr('clip-path'); + + expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#'); + expect(cp.substring(cp.length-1)).toEqual(')'); + }); + + done(); + }); + + }); +}); From d4ae5f40ec6727a466d1be9bc357fe955bd51eb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 4 May 2016 14:55:16 -0400 Subject: [PATCH 4/6] fixup --- test/jasmine/tests/cartesian_test.js | 4 +++- test/jasmine/tests/drawing_test.js | 9 ++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/cartesian_test.js b/test/jasmine/tests/cartesian_test.js index 0893598a3ff..fca848e6f2b 100644 --- a/test/jasmine/tests/cartesian_test.js +++ b/test/jasmine/tests/cartesian_test.js @@ -98,7 +98,7 @@ describe('plot svg clip paths', function() { // https://github.com/angular/angular.js/issues/8934 // append with href - d3.select('body') + var base = d3.select('body') .append('base') .attr('href', 'https://plot.ly'); @@ -117,5 +117,7 @@ describe('plot svg clip paths', function() { done(); }); + base.remove(); + }); }); diff --git a/test/jasmine/tests/drawing_test.js b/test/jasmine/tests/drawing_test.js index 9bb648715f9..0394715de5e 100644 --- a/test/jasmine/tests/drawing_test.js +++ b/test/jasmine/tests/drawing_test.js @@ -11,6 +11,11 @@ describe('Drawing.setClipUrl', function() { this.g = this.svg.append('g'); }); + afterEach(function() { + this.svg.remove(); + this.g.remove(); + }); + it('should set the clip-path attribute', function() { expect(this.g.attr('clip-path')).toBe(null); @@ -30,7 +35,7 @@ describe('Drawing.setClipUrl', function() { it('should append window URL to clip-path if is present', function() { // append with href - d3.select('body') + var base = d3.select('body') .append('base') .attr('href', 'https://plot.ly'); @@ -41,5 +46,7 @@ describe('Drawing.setClipUrl', function() { expect(this.g.attr('clip-path')) .toEqual('url(' + href + '#id3)'); + + base.remove(); }); }); From e80602f3701c96788d4af2036f4fa72f136dd5d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 5 May 2016 11:20:47 -0400 Subject: [PATCH 5/6] make sure to remove being test done() --- test/jasmine/tests/cartesian_test.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/jasmine/tests/cartesian_test.js b/test/jasmine/tests/cartesian_test.js index fca848e6f2b..36829fbe7c6 100644 --- a/test/jasmine/tests/cartesian_test.js +++ b/test/jasmine/tests/cartesian_test.js @@ -85,7 +85,7 @@ describe('plot svg clip paths', function() { var cp = d3.select(this).attr('clip-path'); expect(cp.substring(0, 5)).toEqual('url(#'); - expect(cp.substring(cp.length-1)).toEqual(')'); + expect(cp.substring(cp.length - 1)).toEqual(')'); }); done(); @@ -111,13 +111,11 @@ describe('plot svg clip paths', function() { var cp = d3.select(this).attr('clip-path'); expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#'); - expect(cp.substring(cp.length-1)).toEqual(')'); + expect(cp.substring(cp.length - 1)).toEqual(')'); }); + base.remove(); done(); }); - - base.remove(); - }); }); From b9f4160cb04129f4c034157b2957493e9ee1709e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 5 May 2016 12:07:24 -0400 Subject: [PATCH 6/6] move clip paths tests to plot_interact suite: - for some reason the click paths tests were messing the state in such a way to make a double-click test fail (in click_test.js) on the CircleCI FF38. - by moving those tests to plot_interact (as 'p' > 'c'), the problem is put to the side. --- test/jasmine/tests/cartesian_test.js | 70 ------------------------ test/jasmine/tests/plot_interact_test.js | 70 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 70 deletions(-) diff --git a/test/jasmine/tests/cartesian_test.js b/test/jasmine/tests/cartesian_test.js index 36829fbe7c6..38c09608cef 100644 --- a/test/jasmine/tests/cartesian_test.js +++ b/test/jasmine/tests/cartesian_test.js @@ -49,73 +49,3 @@ describe('zoom box element', function() { .toEqual(0); }); }); - -describe('plot svg clip paths', function() { - - // plot with all features that rely on clip paths - function plot() { - return Plotly.plot(createGraphDiv(), [{ - type: 'contour', - z: [[1,2,3], [2,3,1]] - }, { - type: 'scatter', - y: [2, 1, 2] - }], { - showlegend: true, - xaxis: { - rangeslider: {} - }, - shapes: [{ - xref: 'x', - yref: 'y', - x0: 0, - y0: 0, - x1: 3, - y1: 3 - }] - }); - } - - afterEach(destroyGraphDiv); - - it('should set clip path url to ids (base case)', function(done) { - plot().then(function() { - - d3.selectAll('[clip-path]').each(function() { - var cp = d3.select(this).attr('clip-path'); - - expect(cp.substring(0, 5)).toEqual('url(#'); - expect(cp.substring(cp.length - 1)).toEqual(')'); - }); - - done(); - }); - }); - - it('should set clip path url to ids appended to window url', function(done) { - - // this case occurs in some past versions of AngularJS - // https://github.com/angular/angular.js/issues/8934 - - // append with href - var base = d3.select('body') - .append('base') - .attr('href', 'https://plot.ly'); - - // grab window URL - var href = window.location.href; - - plot().then(function() { - - d3.selectAll('[clip-path]').each(function() { - var cp = d3.select(this).attr('clip-path'); - - expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#'); - expect(cp.substring(cp.length - 1)).toEqual(')'); - }); - - base.remove(); - done(); - }); - }); -}); diff --git a/test/jasmine/tests/plot_interact_test.js b/test/jasmine/tests/plot_interact_test.js index 8748b9d9bb2..9ca7f2b3f2f 100644 --- a/test/jasmine/tests/plot_interact_test.js +++ b/test/jasmine/tests/plot_interact_test.js @@ -436,3 +436,73 @@ describe('Test plot structure', function() { }); }); }); + +describe('plot svg clip paths', function() { + + // plot with all features that rely on clip paths + function plot() { + return Plotly.plot(createGraphDiv(), [{ + type: 'contour', + z: [[1,2,3], [2,3,1]] + }, { + type: 'scatter', + y: [2, 1, 2] + }], { + showlegend: true, + xaxis: { + rangeslider: {} + }, + shapes: [{ + xref: 'x', + yref: 'y', + x0: 0, + y0: 0, + x1: 3, + y1: 3 + }] + }); + } + + afterEach(destroyGraphDiv); + + it('should set clip path url to ids (base case)', function(done) { + plot().then(function() { + + d3.selectAll('[clip-path]').each(function() { + var cp = d3.select(this).attr('clip-path'); + + expect(cp.substring(0, 5)).toEqual('url(#'); + expect(cp.substring(cp.length - 1)).toEqual(')'); + }); + + done(); + }); + }); + + it('should set clip path url to ids appended to window url', function(done) { + + // this case occurs in some past versions of AngularJS + // https://github.com/angular/angular.js/issues/8934 + + // append with href + var base = d3.select('body') + .append('base') + .attr('href', 'https://plot.ly'); + + // grab window URL + var href = window.location.href; + + plot().then(function() { + + d3.selectAll('[clip-path]').each(function() { + var cp = d3.select(this).attr('clip-path'); + + expect(cp.substring(0, 5 + href.length)).toEqual('url(' + href + '#'); + expect(cp.substring(cp.length - 1)).toEqual(')'); + }); + + base.remove(); + done(); + }); + }); +});