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.~~

Fix NaN positions for shapes when dragging them.

abe1cc6

my-tien marked this pull request as draft

July 16, 2025 15:28

archmoj reviewed

Jul 16, 2025

View reviewed changes

src/components/shapes/draw_newshape/newshapes.js

		}else{
		modifyItem('y0',afterEdit.y0);
		modifyItem('y1',afterEdit.y1);
		}

Copy link

Contributor

archmojJul 16, 2025

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:

Is a similar fix needed for the case ofpath?
Could you please add a test here:
plotly.js/test/jasmine/tests/draw_newshape_test.js
Line 1433 in79559af
describe('Activate and edit editable shapes',function(){
?

Copy link

ContributorAuthor

my-tienJul 17, 2025•
edited
Loading

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

ContributorAuthor

my-tienJul 17, 2025

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?

archmoj added the regressionthis used to work label

Jul 16, 2025

my-tien added3 commits

July 17, 2025 13:10

- Remove r2c call. Unnecessary, since category references seem to hav…

9cd5262

…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

Fix formatting for category string

2f2c4a0

WIP draw_newshape_test

13e3cc1

Copy link

ContributorAuthor

my-tien commentedJul 17, 2025•
edited
Loading

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 marked this pull request as ready for review

July 17, 2025 13:14

archmoj reviewed

Jul 17, 2025

View reviewed changes

src/components/shapes/draw_newshape/newshapes.js OutdatedShow resolvedHide resolved

Use .charAt(0) === 'x'/'y' to test for xref/yref pointing to an axis.

c996699

(as is being done at other locations to test this)

archmoj reviewed

Jul 17, 2025

View reviewed changes

test/jasmine/tests/draw_newshape_test.js OutdatedShow resolvedHide resolved

archmoj reviewed

Jul 17, 2025

View reviewed changes

test/jasmine/tests/draw_newshape_test.js OutdatedShow resolvedHide resolved

archmoj reviewed

Jul 17, 2025

View reviewed changes

test/jasmine/tests/draw_newshape_test.js OutdatedShow resolvedHide resolved

Copy link

Contributor

archmoj commentedJul 17, 2025

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

gvwilson assignedmy-tien

Jul 18, 2025

gvwilson added community

community contribution

needed for current cycle

fixfixes something broken labels

Jul 18, 2025

Merge remote-tracking branch 'origin-plotly/master' into fix-edit-shapes

5fa4692

Copy link

Contributor

archmoj commentedJul 18, 2025

@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 added2 commits

July 21, 2025 09:47

Add jasmine tests for dragging shape attached to date-axis

58c811a

(thx Mojtaba, for the code)

Merge remote-tracking branch 'origin-plotly/master' into fix-edit-shapes

ee3106f

Copy link

ContributorAuthor

my-tien commentedJul 21, 2025

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

Copy link

Contributor

archmoj commentedJul 21, 2025

@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!

Add draftlog for PR 7470.

7cce307

archmoj approved these changes

Jul 21, 2025

View reviewed changes

Copy link

Contributor

archmoj left a comment

Choose a reason for hiding this comment

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

💃

archmoj merged commit4bd8b85 intoplotly:master

Jul 21, 2025

5 checks passed

Labels

community

community contribution

fix

fixes something broken

needed for current cycle

regression

this used to work

Movatterモバイル変換

Uh oh!

Fix NaN positions for shapes when dragging them.#7470

Fix NaN positions for shapes when dragging them.#7470

Uh oh!

Conversation

my-tien commentedJul 16, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

archmojJul 16, 2025

Choose a reason for hiding this comment

Uh oh!

my-tienJul 17, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

my-tienJul 17, 2025

Choose a reason for hiding this comment

Uh oh!

my-tien commentedJul 17, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

archmoj commentedJul 17, 2025

Uh oh!

archmoj commentedJul 18, 2025

Uh oh!

my-tien commentedJul 21, 2025

Uh oh!

archmoj commentedJul 21, 2025

Uh oh!

archmoj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

my-tien commentedJul 16, 2025•
edited
Loading

my-tienJul 17, 2025•
edited
Loading

my-tien commentedJul 17, 2025•
edited
Loading