From 3e96a409896dad8aeb3fb488a4a07cf5c283b18a Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 4 May 2020 10:40:33 -0400 Subject: [PATCH 1/3] fix issue 4806 and 4808 - use and ensure stroke event only to activate editable shapes --- src/components/shapes/draw.js | 17 +-- .../shapes/draw_newshape/attributes.js | 11 +- test/jasmine/tests/draw_newshape_test.js | 130 +++++++++++++++++- 3 files changed, 139 insertions(+), 19 deletions(-) diff --git a/src/components/shapes/draw.js b/src/components/shapes/draw.js index 28e7f849f74..d0438fcb8f8 100644 --- a/src/components/shapes/draw.js +++ b/src/components/shapes/draw.js @@ -119,6 +119,11 @@ function drawOne(gd, index) { var lineColor = options.line.width ? options.line.color : 'rgba(0,0,0,0)'; var lineWidth = options.line.width; var lineDash = options.line.dash; + if(!lineWidth && options.editable === true) { + // ensure invisible border to active the shape + lineWidth = 5; + lineDash = 'solid'; + } var isOpen = d[d.length - 1] !== 'Z'; @@ -163,15 +168,11 @@ function drawOne(gd, index) { } else { if(gd._context.edits.shapePosition) { setupDragElement(gd, path, options, index, shapeLayer, editHelpers); + } else { + if(options.editable === true) { + path.style('pointer-events', 'stroke'); + } } - - path.style('pointer-events', - !couldHaveActiveShape(gd) || ( - lineWidth < 2 || ( // not has a remarkable border - !isOpen && Color.opacity(fillColor) * opacity > 0.5 // not too transparent - ) - ) ? 'all' : 'stroke' - ); } path.node().addEventListener('click', function() { return activateShape(gd, path); }); diff --git a/src/components/shapes/draw_newshape/attributes.js b/src/components/shapes/draw_newshape/attributes.js index 90d6cb810e5..9a872009edc 100644 --- a/src/components/shapes/draw_newshape/attributes.js +++ b/src/components/shapes/draw_newshape/attributes.js @@ -44,12 +44,7 @@ module.exports = { dflt: 'rgba(0,0,0,0)', role: 'info', editType: 'none', - description: [ - 'Sets the color filling new shapes\' interior.', - 'Please note that if using a fillcolor with alpha greater than half,', - 'drag inside the active shape starts moving the shape underneath,', - 'otherwise a new shape could be started over.' - ].join(' ') + description: 'Sets the color filling new shapes\' interior.' }, fillrule: { valType: 'enumerated', @@ -99,6 +94,10 @@ module.exports = { }, activeshape: { + description: [ + 'An \'editable`\' shape can be \'activated\' by clicking on its border line.' + ].join(' '), + fillcolor: { valType: 'color', dflt: 'rgb(255,0,255)', diff --git a/test/jasmine/tests/draw_newshape_test.js b/test/jasmine/tests/draw_newshape_test.js index 83c10f8b505..6ce90413e61 100644 --- a/test/jasmine/tests/draw_newshape_test.js +++ b/test/jasmine/tests/draw_newshape_test.js @@ -1089,7 +1089,7 @@ describe('Activate and edit editable shapes', function() { afterEach(destroyGraphDiv); ['mouse'].forEach(function(device) { - it('@flaky reactangle using' + device, function(done) { + it('@flaky reactangle using ' + device, function(done) { var i = 0; // shape index Plotly.newPlot(gd, { @@ -1224,7 +1224,7 @@ describe('Activate and edit editable shapes', function() { .then(done); }); - it('@flaky circle using' + device, function(done) { + it('@flaky circle using ' + device, function(done) { var i = 1; // shape index Plotly.newPlot(gd, { @@ -1281,7 +1281,7 @@ describe('Activate and edit editable shapes', function() { .then(done); }); - it('@flaky closed-path using' + device, function(done) { + it('@flaky closed-path using ' + device, function(done) { var i = 2; // shape index Plotly.newPlot(gd, { @@ -1326,7 +1326,7 @@ describe('Activate and edit editable shapes', function() { .then(done); }); - it('@flaky bezier curves using' + device, function(done) { + it('@flaky bezier curves using ' + device, function(done) { var i = 5; // shape index Plotly.newPlot(gd, { @@ -1371,7 +1371,7 @@ describe('Activate and edit editable shapes', function() { .then(done); }); - it('@flaky multi-cell path using' + device, function(done) { + it('@flaky multi-cell path using ' + device, function(done) { var i = 6; // shape index Plotly.newPlot(gd, { @@ -1426,3 +1426,123 @@ describe('Activate and edit editable shapes', function() { }); }); }); + + +describe('Activate and edit editable shapes', function() { + var gd; + + beforeEach(function() { + gd = createGraphDiv(); + }); + + afterEach(destroyGraphDiv); + + it('should not set pointer-events for non-editable shapes i.e. to allow pan/zoom/hover work inside shapes', function(done) { + Plotly.newPlot(gd, { + data: [{ + mode: 'markers', + x: [1, 3, 3], + y: [2, 1, 3] + }], + layout: { + shapes: [ + { + x0: 1.5, + x1: 2.5, + y0: 1.5, + y1: 2.5, + fillcolor: 'blue' + } + ] + } + }) + + .then(function() { + var el = Plotly.d3.selectAll('.shapelayer path')[0][0]; + var pointerEvents = el.style['pointer-events']; + expect(pointerEvents).not.toBe('all'); + expect(pointerEvents).not.toBe('stroke'); + expect(pointerEvents).toBe(''); + }) + + .catch(failTest) + .then(done); + }); + + it('should provide invisible border & set pointer-events to "stroke" for editable shapes i.e. to allow shape activation', function(done) { + Plotly.newPlot(gd, { + data: [{ + mode: 'markers', + x: [1, 3, 3], + y: [2, 1, 3] + }], + layout: { + shapes: [ + { + editable: true, + x0: 1.5, + x1: 2.5, + y0: 1.5, + y1: 2.5, + fillcolor: 'blue', + line: { + width: 0, + dash: 'dash', + color: 'black' + } + } + ] + } + }) + + .then(function() { + var el = Plotly.d3.selectAll('.shapelayer path')[0][0]; + expect(el.style['pointer-events']).toBe('stroke'); + expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color + expect(el.style['stroke-opacity']).toBe('0'); // invisible + expect(el.style['stroke-width']).toBe('5px'); // some pixels to activate shape + }) + + .catch(failTest) + .then(done); + }); + + it('should not provide invisible border & set pointer-events to "stroke" for shapes made editable via config', function(done) { + Plotly.newPlot(gd, { + data: [{ + mode: 'markers', + x: [1, 3, 3], + y: [2, 1, 3] + }], + layout: { + shapes: [ + { + x0: 1.5, + x1: 2.5, + y0: 1.5, + y1: 2.5, + fillcolor: 'blue', + line: { + width: 0, + dash: 'dash', + color: 'black' + } + } + ] + }, onfig: { + editable: true + } + }) + + .then(function() { + var el = Plotly.d3.selectAll('.shapelayer path')[0][0]; + expect(el.style['pointer-events']).toBe(''); + expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color + expect(el.style['stroke-opacity']).toBe('0'); // invisible + expect(el.style['stroke-width']).toBe('0px'); // no extra pixels + }) + + .catch(failTest) + .then(done); + }); +}); From 078c007652bb4458c3d7a5ce1a61302403fc4dca Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 4 May 2020 16:04:09 -0400 Subject: [PATCH 2/3] allow activation of editable shapes using opaque fill --- src/components/shapes/draw.js | 10 +- .../shapes/draw_newshape/attributes.js | 11 +- test/jasmine/tests/draw_newshape_test.js | 128 ++++++++++++++++-- 3 files changed, 131 insertions(+), 18 deletions(-) diff --git a/src/components/shapes/draw.js b/src/components/shapes/draw.js index d0438fcb8f8..b519a654a8b 100644 --- a/src/components/shapes/draw.js +++ b/src/components/shapes/draw.js @@ -120,7 +120,7 @@ function drawOne(gd, index) { var lineWidth = options.line.width; var lineDash = options.line.dash; if(!lineWidth && options.editable === true) { - // ensure invisible border to active the shape + // ensure invisible border to activate the shape lineWidth = 5; lineDash = 'solid'; } @@ -168,10 +168,10 @@ function drawOne(gd, index) { } else { if(gd._context.edits.shapePosition) { setupDragElement(gd, path, options, index, shapeLayer, editHelpers); - } else { - if(options.editable === true) { - path.style('pointer-events', 'stroke'); - } + } else if(options.editable === true) { + path.style('pointer-events', + (isOpen || Color.opacity(fillColor) * opacity <= 0.5) ? 'stroke' : 'all' + ); } } diff --git a/src/components/shapes/draw_newshape/attributes.js b/src/components/shapes/draw_newshape/attributes.js index 9a872009edc..90d6cb810e5 100644 --- a/src/components/shapes/draw_newshape/attributes.js +++ b/src/components/shapes/draw_newshape/attributes.js @@ -44,7 +44,12 @@ module.exports = { dflt: 'rgba(0,0,0,0)', role: 'info', editType: 'none', - description: 'Sets the color filling new shapes\' interior.' + description: [ + 'Sets the color filling new shapes\' interior.', + 'Please note that if using a fillcolor with alpha greater than half,', + 'drag inside the active shape starts moving the shape underneath,', + 'otherwise a new shape could be started over.' + ].join(' ') }, fillrule: { valType: 'enumerated', @@ -94,10 +99,6 @@ module.exports = { }, activeshape: { - description: [ - 'An \'editable`\' shape can be \'activated\' by clicking on its border line.' - ].join(' '), - fillcolor: { valType: 'color', dflt: 'rgb(255,0,255)', diff --git a/test/jasmine/tests/draw_newshape_test.js b/test/jasmine/tests/draw_newshape_test.js index 6ce90413e61..3b146e072b1 100644 --- a/test/jasmine/tests/draw_newshape_test.js +++ b/test/jasmine/tests/draw_newshape_test.js @@ -1447,11 +1447,52 @@ describe('Activate and edit editable shapes', function() { layout: { shapes: [ { + editable: false, x0: 1.5, + x1: 2, + y0: 1.5, + y1: 2, + opacity: 0.5, + fillcolor: 'blue', + line: { + width: 0, + dash: 'dash', + color: 'black' + } + }, { + editable: false, + x0: 2, x1: 2.5, y0: 1.5, + y1: 2, + opacity: 0.7, + fillcolor: 'rgba(255,0,0,0.7)', // N.B. 0.7 * 0.7 = 0.49 <= 0.5 is quite transparent + line: { + width: 0, + dash: 'dash', + color: 'black' + } + }, { + editable: false, + x0: 1.5, + x1: 2, + y0: 2, y1: 2.5, - fillcolor: 'blue' + fillcolor: 'red', + line: { + width: 3, + dash: 'dash', + color: 'green' + } + }, { + editable: false, + path: 'M2,2H2.5,V2.5', // open path + fillcolor: 'rgba(0,0,0,0)', + line: { + width: 3, + dash: 'dash', + color: 'green' + } } ] } @@ -1459,17 +1500,35 @@ describe('Activate and edit editable shapes', function() { .then(function() { var el = Plotly.d3.selectAll('.shapelayer path')[0][0]; - var pointerEvents = el.style['pointer-events']; - expect(pointerEvents).not.toBe('all'); - expect(pointerEvents).not.toBe('stroke'); - expect(pointerEvents).toBe(''); + expect(el.style['pointer-events']).toBe(''); + expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color + expect(el.style['stroke-opacity']).toBe('0'); // invisible + expect(el.style['stroke-width']).toBe('0px'); // no pixel + + el = Plotly.d3.selectAll('.shapelayer path')[0][1]; + expect(el.style['pointer-events']).toBe(''); + expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color + expect(el.style['stroke-opacity']).toBe('0'); // invisible + expect(el.style['stroke-width']).toBe('0px'); // no pixel + + el = Plotly.d3.selectAll('.shapelayer path')[0][2]; + expect(el.style['pointer-events']).toBe(''); + expect(el.style.stroke).toBe('rgb(0, 128, 0)'); // custom color + expect(el.style['stroke-opacity']).toBe('1'); // visible + expect(el.style['stroke-width']).toBe('3px'); // custom pixel + + el = Plotly.d3.selectAll('.shapelayer path')[0][3]; + expect(el.style['pointer-events']).toBe(''); + expect(el.style.stroke).toBe('rgb(0, 128, 0)'); // custom color + expect(el.style['stroke-opacity']).toBe('1'); // visible + expect(el.style['stroke-width']).toBe('3px'); // custom pixel }) .catch(failTest) .then(done); }); - it('should provide invisible border & set pointer-events to "stroke" for editable shapes i.e. to allow shape activation', function(done) { + it('should provide invisible border & set pointer-events (depending on fill transparency) for editable shapes i.e. to allow shape activation', function(done) { Plotly.newPlot(gd, { data: [{ mode: 'markers', @@ -1481,15 +1540,50 @@ describe('Activate and edit editable shapes', function() { { editable: true, x0: 1.5, - x1: 2.5, + x1: 2, y0: 1.5, - y1: 2.5, + y1: 2, + opacity: 0.5, fillcolor: 'blue', line: { width: 0, dash: 'dash', color: 'black' } + }, { + editable: true, + x0: 2, + x1: 2.5, + y0: 1.5, + y1: 2, + opacity: 0.7, + fillcolor: 'rgba(255,0,0,0.7)', // N.B. 0.7 * 0.7 = 0.49 <= 0.5 is quite transparent + line: { + width: 0, + dash: 'dash', + color: 'black' + } + }, { + editable: true, + x0: 1.5, + x1: 2, + y0: 2, + y1: 2.5, + fillcolor: 'red', + line: { + width: 3, + dash: 'dash', + color: 'green' + } + }, { + editable: true, + path: 'M2,2H2.5,V2.5', // open path + fillcolor: 'rgba(0,0,0,0)', + line: { + width: 3, + dash: 'dash', + color: 'green' + } } ] } @@ -1501,6 +1595,24 @@ describe('Activate and edit editable shapes', function() { expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color expect(el.style['stroke-opacity']).toBe('0'); // invisible expect(el.style['stroke-width']).toBe('5px'); // some pixels to activate shape + + el = Plotly.d3.selectAll('.shapelayer path')[0][1]; + expect(el.style['pointer-events']).toBe('stroke'); + expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color + expect(el.style['stroke-opacity']).toBe('0'); // invisible + expect(el.style['stroke-width']).toBe('5px'); // some pixels to activate shape + + el = Plotly.d3.selectAll('.shapelayer path')[0][2]; + expect(el.style['pointer-events']).toBe('all'); + expect(el.style.stroke).toBe('rgb(0, 128, 0)'); // custom color + expect(el.style['stroke-opacity']).toBe('1'); // visible + expect(el.style['stroke-width']).toBe('3px'); // custom pixel + + el = Plotly.d3.selectAll('.shapelayer path')[0][3]; + expect(el.style['pointer-events']).toBe('stroke'); + expect(el.style.stroke).toBe('rgb(0, 128, 0)'); // custom color + expect(el.style['stroke-opacity']).toBe('1'); // visible + expect(el.style['stroke-width']).toBe('3px'); // custom pixel }) .catch(failTest) From bb3f85800d7e8a6d4bc05d93d2f1e8720b397eef Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 4 May 2020 17:07:25 -0400 Subject: [PATCH 3/3] fixup config editable: true test --- test/jasmine/tests/draw_newshape_test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/draw_newshape_test.js b/test/jasmine/tests/draw_newshape_test.js index 3b146e072b1..e81b969f6b0 100644 --- a/test/jasmine/tests/draw_newshape_test.js +++ b/test/jasmine/tests/draw_newshape_test.js @@ -1641,14 +1641,15 @@ describe('Activate and edit editable shapes', function() { } } ] - }, onfig: { + }, + config: { editable: true } }) .then(function() { var el = Plotly.d3.selectAll('.shapelayer path')[0][0]; - expect(el.style['pointer-events']).toBe(''); + expect(el.style['pointer-events']).toBe('all'); expect(el.style.stroke).toBe('rgb(0, 0, 0)'); // no color expect(el.style['stroke-opacity']).toBe('0'); // invisible expect(el.style['stroke-width']).toBe('0px'); // no extra pixels