From ffb67f9d239a126b42890f4382841616ae9777a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 17 Sep 2019 14:49:48 -0400 Subject: [PATCH 1/4] fix #4194 - stringify locations[i] for hover pointData - so the rest of Fx.hover can properly handle it, the bug currently only happens for number locations[i] that make it part of the hover label content. --- src/traces/choropleth/hover.js | 5 +-- test/jasmine/tests/choroplethmapbox_test.js | 38 +++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/traces/choropleth/hover.js b/src/traces/choropleth/hover.js index ca19552ea50..d6752372606 100644 --- a/src/traces/choropleth/hover.js +++ b/src/traces/choropleth/hover.js @@ -58,6 +58,7 @@ function makeHoverInfo(pointData, trace, pt) { if(trace.hovertemplate) return; var hoverinfo = pt.hi || trace.hoverinfo; + var loc = String(pt.loc); var parts = (hoverinfo === 'all') ? attributes.hoverinfo.flags : @@ -72,10 +73,10 @@ function makeHoverInfo(pointData, trace, pt) { var text = []; if(hasIdAsNameLabel) { - pointData.nameOverride = pt.loc; + pointData.nameOverride = loc; } else { if(hasName) pointData.nameOverride = trace.name; - if(hasLocation) text.push(pt.loc); + if(hasLocation) text.push(loc); } if(hasZ) { diff --git a/test/jasmine/tests/choroplethmapbox_test.js b/test/jasmine/tests/choroplethmapbox_test.js index 68293b2e8d1..41657a1181a 100644 --- a/test/jasmine/tests/choroplethmapbox_test.js +++ b/test/jasmine/tests/choroplethmapbox_test.js @@ -589,6 +589,44 @@ describe('@noCI Test choroplethmapbox hover:', function() { nums: '10.000', name: 'PROP::New York', evtPts: [{location: 'NY', z: 10, pointNumber: 0, curveNumber: 0, properties: {name: 'New York'}}] + }, { + desc: 'with "typeof number" locations[i] and feature id (in *name* label case)', + patch: function() { + var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json')); + fig.data.shift(); + fig.data[0].locations = [100]; + fig.data[0].geojson.id = 100; + return fig; + }, + nums: '10', + name: '100', + evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}] + }, { + desc: 'with "typeof number" locations[i] and feature id (in *nums* label case)', + patch: function() { + var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json')); + fig.data.shift(); + fig.data[0].locations = [100]; + fig.data[0].geojson.id = 100; + fig.data[0].hoverinfo = 'location+name'; + return fig; + }, + nums: '100', + name: 'trace 0', + evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}] + }, { + desc: 'with "typeof number" locations[i] and feature id (hovertemplate case)', + patch: function() { + var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json')); + fig.data.shift(); + fig.data[0].locations = [100]; + fig.data[0].geojson.id = 100; + fig.data[0].hovertemplate = '### %{location}%{location} ###'; + return fig; + }, + nums: '### 100', + name: '100 ###', + evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}] }]; specs.forEach(function(s) { From c18fd75c413b2712d8104a5cb7830f4d5be25b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 17 Sep 2019 16:28:51 -0400 Subject: [PATCH 2/4] add clearPromiseQueue to Plotly.react ... on-par with Plotly.restyle, Plotly.relayout and Plotly.update --- src/plot_api/plot_api.js | 1 + test/jasmine/tests/mapbox_test.js | 34 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 241f437ca3b..09d84499e7b 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2657,6 +2657,7 @@ function react(gd, data, layout, config) { function addFrames() { return exports.addFrames(gd, frames); } gd = Lib.getGraphDiv(gd); + helpers.clearPromiseQueue(gd); var oldFullData = gd._fullData; var oldFullLayout = gd._fullLayout; diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index 7bc209242cc..31362613642 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -942,6 +942,40 @@ describe('@noCI, mapbox plots', function() { .then(done); }, LONG_TIMEOUT_INTERVAL); + it('@gl should not wedge graph after reacting to invalid layer', function(done) { + Plotly.react(gd, [{type: 'scattermapbox'}], { + mapbox: { + layers: [{ source: 'invalid' }] + } + }) + .then(function() { + fail('The above Plotly.react promise should be rejected'); + }) + .catch(function() { + expect(gd._promises.length).toBe(1, 'has 1 rejected promise in queue'); + }) + .then(function() { + return Plotly.react(gd, [{type: 'scattermapbox'}], { + mapbox: { + layers: [{ + sourcetype: 'vector', + sourcelayer: 'contour', + source: 'mapbox://mapbox.mapbox-terrain-v2' + }] + } + }); + }) + .then(function() { + expect(gd._promises.length).toBe(0, 'rejected promise has been cleared'); + + var mapInfo = getMapInfo(gd); + expect(mapInfo.layoutLayers.length).toBe(1, 'one layer'); + expect(mapInfo.layoutSources.length).toBe(1, 'one layer source'); + }) + .catch(failTest) + .then(done); + }, LONG_TIMEOUT_INTERVAL); + it('@gl should be able to update the access token', function(done) { Plotly.relayout(gd, 'mapbox.accesstoken', 'wont-work').catch(function(err) { expect(gd._fullLayout.mapbox.accesstoken).toEqual('wont-work'); From e26dfc3710c9eca86192e801029ebd108d6e7a38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 17 Sep 2019 16:29:51 -0400 Subject: [PATCH 3/4] do not attempt to remove non-existant source and layers - this can happen when trying to remove a `visible:false` layer --- src/plots/mapbox/layers.js | 4 ++-- test/jasmine/tests/mapbox_test.js | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/plots/mapbox/layers.js b/src/plots/mapbox/layers.js index 16022cd4716..f9791dd0ed4 100644 --- a/src/plots/mapbox/layers.js +++ b/src/plots/mapbox/layers.js @@ -145,8 +145,8 @@ proto.removeLayer = function() { proto.dispose = function() { var map = this.subplot.map; - map.removeLayer(this.idLayer); - map.removeSource(this.idSource); + if(map.getLayer(this.idLayer)) map.removeLayer(this.idLayer); + if(map.getSource(this.idSource)) map.removeSource(this.idSource); }; function isVisible(opts) { diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index 31362613642..c7675abb415 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -976,6 +976,28 @@ describe('@noCI, mapbox plots', function() { .then(done); }, LONG_TIMEOUT_INTERVAL); + it('@gl should not attempt to remove non-existing layer sources', function(done) { + function _assert(msg, exp) { + return function() { + var layerList = gd._fullLayout.mapbox._subplot.layerList; + expect(layerList.length).toBe(exp, msg); + }; + } + + Plotly.react(gd, [{type: 'scattermapbox'}], { + mapbox: { layers: [{}] } + }) + .then(_assert('1 visible:false layer', 1)) + .then(function() { + return Plotly.react(gd, [{type: 'scattermapbox'}], { + mapbox: { layers: [] } + }); + }) + .then(_assert('no layers', 0)) + .catch(failTest) + .then(done); + }, LONG_TIMEOUT_INTERVAL); + it('@gl should be able to update the access token', function(done) { Plotly.relayout(gd, 'mapbox.accesstoken', 'wont-work').catch(function(err) { expect(gd._fullLayout.mapbox.accesstoken).toEqual('wont-work'); From 3181cbe8f30562f8e06c7db7436fd6e405edc779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 17 Sep 2019 16:30:53 -0400 Subject: [PATCH 4/4] ensure that all tile source items are non-empty strings --- src/plots/mapbox/layers.js | 17 +++++++++++++---- test/jasmine/tests/mapbox_test.js | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/plots/mapbox/layers.js b/src/plots/mapbox/layers.js index f9791dd0ed4..dd47dcd188f 100644 --- a/src/plots/mapbox/layers.js +++ b/src/plots/mapbox/layers.js @@ -150,12 +150,21 @@ proto.dispose = function() { }; function isVisible(opts) { + if(!opts.visible) return false; + var source = opts.source; - return opts.visible && ( - Lib.isPlainObject(source) || - ((typeof source === 'string' || Array.isArray(source)) && source.length > 0) - ); + if(Array.isArray(source) && source.length > 0) { + for(var i = 0; i < source.length; i++) { + if(typeof source[i] !== 'string' || source[i].length === 0) { + return false; + } + } + return true; + } + + return Lib.isPlainObject(source) || + (typeof source === 'string' && source.length > 0); } function convertOpts(opts) { diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index c7675abb415..4981fc37bf2 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -998,6 +998,24 @@ describe('@noCI, mapbox plots', function() { .then(done); }, LONG_TIMEOUT_INTERVAL); + it('@gl should validate layout layer input', function(done) { + Plotly.newPlot(gd, [{type: 'scattermapbox'}], { + mapbox: { + layers: [{ + sourcetype: 'raster', + source: [''] + }] + } + }) + .then(function() { + var mapInfo = getMapInfo(gd); + expect(mapInfo.layoutLayers.length).toBe(0, 'no on-map layer'); + expect(mapInfo.layoutSources.length).toBe(0, 'no map source'); + }) + .catch(failTest) + .then(done); + }, LONG_TIMEOUT_INTERVAL); + it('@gl should be able to update the access token', function(done) { Plotly.relayout(gd, 'mapbox.accesstoken', 'wont-work').catch(function(err) { expect(gd._fullLayout.mapbox.accesstoken).toEqual('wont-work');