Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix NaN positions for shapes when dragging them. - #7470

Merged
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes
Jul 21, 2025
Merged

Fix NaN positions for shapes when dragging them.#7470
archmoj merged 9 commits into
plotly:masterfrom
my-tien:fix-edit-shapes

Conversation

@my-tien

@my-tienmy-tien commented Jul 16, 2025

Copy link
Copy Markdown
Contributor

Fixes#7465

Previously, x0shift, x1shift, y0shift, y1shift or 0 were always applied to x0, x1, y0, y1 when dragging a shape.

However, as per the description of the shift attributes, they should only be applied for (multi-)category axes. Also if x0,... are strings, a conversion to the corresponding category index was missing.

@my-tien
my-tien marked this pull request as draft July 16, 2025 15:28
} else {
modifyItem('y0', afterEdit.y0);
modifyItem('y1', afterEdit.y1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @my-tien for the fix.
Two questions:

  1. Is a similar fix needed for the case of path?
  2. Could you please add a test here:
    describe('Activate and edit editable shapes',function(){
    ?

@my-tienmy-tienJul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look at path and adding a test. However, my fix is not yet correct. Among other things, afterEdit can be a shape with xref and yref paper apparently even if the ref was something different before. I'll have to see, how I can correctly translate the shift variables to paper as well. I will probably push some changes later today.

UPDATE: This is a separate bug. As mentioned further below, I'll open an issue for it later.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the path case the change is not necessary, because the shift properties are ignored if path is used instead of x0, x1, y0, y1.

Regarding the test, I added one with a test case that previously failed and now works. However, I am not sure how to test this locally because for some reason on my machine several Jasmine tests always fail. Could you please look at it on your end and make the necessary adjustments?

@archmojarchmoj added the regression this used to work label Jul 16, 2025
…e been converted to numerical before this point already.
- Don't use plotinfo to retrieve the axis, somehow it doesn't always contain the correct axis information. Use `axis_ids.getFromId` instead.
- Fix formatting
@my-tien

my-tien commented Jul 17, 2025

Copy link
Copy Markdown
ContributorAuthor

While working on this issue, I discovered a bug that existed also before version 2.24: A shape attached to xaxis 2 cannot be dragged. I'll open an issue later.

EDIT: Here is the issue: #7472 (comment)

@my-tien
my-tien marked this pull request as ready for review July 17, 2025 13:14
Comment threadsrc/components/shapes/draw_newshape/newshapes.js Outdated
(as is being done at other locations to test this)
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
Comment threadtest/jasmine/tests/draw_newshape_test.js Outdated
@archmoj

Copy link
Copy Markdown
Contributor

Some CircleCI fixes are added to the master.
Please fetch master and merge it into this branch.
Thank you 🙏

@gvwilsongvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Jul 18, 2025
@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please replace your test with the following tests:

describe('Activate and edit editable shapes - date axes',function(){varfig={data: [{x: [0,50],y: [0,50]}],layout: {width: 800,height: 600,margin: {t: 100,b: 50,l: 100,r: 50},xaxis: {type: 'date',range: ["1975-07-01","2380-07-01"]},yaxis: {range: [301.78041543026706,-18.694362017804156]},shapes: [{editable: true,layer: 'below',type: 'rect',line: {width: 5},fillcolor: 'red',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75},{editable: true,layer: 'top',type: 'circle',line: {width: 5},fillcolor: 'green',opacity: 0.5,xref: 'xaxis',yref: 'yaxis',x0: '2125-01-01',y0: 25,x1: '2175-01-01',y1: 75}]},config: {editable: false,modeBarButtonsToAdd: ['drawline','drawopenpath','drawclosedpath','drawcircle','drawrect','eraseshape']}};vargd;beforeEach(function(){gd=createGraphDiv();});afterEach(destroyGraphDiv);['mouse'].forEach(function(device){it('reactangle using '+device,function(done){vari=0;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// shape between 175, 160 and 255, 230.then(function(){click(200,160);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2025-01-01',y0: 25,x1: '2075-01-01',y1: 75});}).then(function(){drag([[255,230],[300,200]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 24.997032640949552,x1: '2103-01-15 16:20:58.3385',y1: 53.63323442136499});}).then(function(){drag([[300,200],[255,230]]);})// move vertex back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){drag([[215,195],[300,200]]);})// move shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2077-12-16 18:31:40.8',y0: 24.997032640949552,x1: '2127-12-18 16:43:07.5692',y1: 74.99821958456974});}).then(function(){drag([[300,200],[215,195]]);})// move shape back.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('rect');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2024-12-30 20:44:36.1846',y0: 25,x1: '2074-12-31 18:56:02.9538',y1: 75});}).then(function(){click(100,100);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking outside');}).then(function(){click(255,230);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking on corner');}).then(function(){click(215,195);}).then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(undefined,'deactivate shape by clicking inside');}).then(done,done.fail);});it('circle using '+device,function(done){vari=1;// shape indexPlotly.newPlot(gd,{data: fig.data,layout: fig.layout,config: fig.config})// next shape.then(function(){click(355,225);})// activate shape.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'activate shape by clicking border');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2125-01-01',x1: '2175-01-01',y0: 25,y1: 75});}).then(function(){drag([[338,196],[300,175]]);})// move vertex.then(function(){varid=gd._fullLayout._activeShapeIndex;expect(id).toEqual(i,'keep shape active after drag corner');varshapes=gd._fullLayout.shapes;varobj=shapes[id]._input;expect(obj.type).toEqual('circle');print(obj);assertPos({x0: obj.x0,y0: obj.y0,x1: obj.x1,y1: obj.y1},{x0: '2186-11-02 07:04:22.7446',y0: 74.99821958456971,x1: '2113-03-01 18:44:58.3385',y1: 10.04154302670623});}).then(done,done.fail);});});});

@my-tien

Copy link
Copy Markdown
ContributorAuthor

Thanks @archmoj, Master is merged and test is included. It also ran successfully on my machine. 👍

@archmoj

Copy link
Copy Markdown
Contributor

@my-tien Please add a draft log.

- Fix editable shapes (regression introduced in 2.34.0) [[#7470](https://github.com/plotly/plotly.js/pull/7470)],
with thanks to @my-tien for the contribution!

Thank you!

@archmojarchmoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

communitycommunity contributionfixfixes something brokenP1needed for current cycleregressionthis used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Editable shapes seem to be broken going from 2.33.0 to 2.34.0 and beyond

3 participants

@my-tien@archmoj@gvwilson