Skip to content

Conversation

@alexshoe
Copy link
Contributor

@alexshoe alexshoe commented Dec 4, 2025

Description:

Extends xref and yref to accept arrays, allowing shapes to span multiple subplots with each vertex anchored to a different axis. See #7151 for more information.

Example:

shapes: [{  
  type: 'rect',  
  x0: 0, 
  x1: 1, 
  y0: 0, 
  y1: 1,  
  xref: ['x', 'x2'],  // x0 uses 'x', x1 uses 'x2'  
  yref: ['y', 'y2']   // y0 uses 'y', y1 uses 'y2'
}]

Progress:

  • Step 1: Attributes & validation
    • Extend xref and yref to allow array values
    • Add helper function to check number of defining coordinates of a shape
    • Update value validation to work with array values
  • Step 2: Refactor coordinate conversion
  • Step 3: Update shape rendering
  • Step 4: Addressing editable shapes
  • Step 5: Tests + documentation

@alexshoe alexshoe marked this pull request as draft December 4, 2025 22:16
Comment on lines 66 to 70
// for each path command, check if there is a drawn coordinate
var segmentType = segment.charAt(0);
var hasDrawnX = constants.paramIsX[segmentType].drawn !== undefined;
var hasDrawnY = constants.paramIsY[segmentType].drawn !== undefined;
if(hasDrawnX || hasDrawnY) coordCount++;
Copy link
Contributor

@emilykl emilykl Dec 4, 2025

Choose a reason for hiding this comment

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

Hmm. H and V segments cause an odd usability issue I hadn't considered: if a path contains H and V segments then the expected lengths of xref and yref may be different, and the corresponding list items will be "out of sync" with respect to which path segments they correspond to (i.e. it will not always be the case that xref[n] and yref[n] refer to the same path segment).

For example here is a rectangular path: M0,0H10V5H0Z (see visualization)

There are four segments (not counting Z), but only 3 x-values and 2 y-values are needed to define the shape. So you would have:

{
    "path": "M0,0H10V5H0Z",
    "xref": ["x", "x2", "x"],
    "yref": ["y", "y2"],
}

which is a bit odd, and sort a high cognitive load for the user to figure out the right xref and yref lists.

That said, it seems probably better than the other alternative that comes to mind, which would be to force the user to add an xref and a yref for every single segment, even if it will be ignored for some segments.

}
}

containerOut[refAttr] = axRef;
Copy link
Contributor

Choose a reason for hiding this comment

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

The core Lib.coerce() function uses more complex logic here for setting the attribute in the out container -- I'm not 100% sure whether that's needed here, but something to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just documenting that we chatted about this and decided we'd look deeper into Lib.coerce + if it's alright to not call it here.

@alexshoe alexshoe marked this pull request as ready for review January 1, 2026 19:17
@alexshoe alexshoe requested a review from emilykl January 1, 2026 19:17
xref: {
arrayOk: true,
valType: 'enumerated',
values: [
Copy link
Contributor

@emilykl emilykl Jan 7, 2026

Choose a reason for hiding this comment

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

@alexshoe Is it not possible to continue using extendFlat({}, annAttrs.xref, {...}) here?

That would save us from redefining valType, values, editType etc. which are the same as in annAttrs.xref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right, I didn't realize I could override certain parameters when using extendFlat so I thought I would have to use the description from annAttrs.xref as well. I'll make that change.

@alexshoe alexshoe linked an issue Jan 13, 2026 that may be closed by this pull request
axisPlaceableObjs.axisRefDescription('x', 'left', 'right')
axisPlaceableObjs.axisRefDescription('x', 'left', 'right'),
'If an array of axis IDs is provided, each `x` value will refer to the corresponding axis',
'(e.g., [\'x\', \'x2\'] for a rectangle means `x0` uses the `x` axis and `x1` uses the `x2` axis).',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be explicit how this applies to each shape type - and in particular that it applies to paths

Suggested change
'(e.g., [\'x\', \'x2\'] for a rectangle means `x0` uses the `x` axis and `x1` uses the `x2` axis).',
'e.g., [\'x\', \'x2\'] for a rectangle, line, or circle means `x0` uses the `x` axis and `x1` uses the `x2` axis.',
'Path shapes using an array should have one entry for each x coordinate in the string.',


if(Array.isArray(inputRef) && inputRef.length > 0) {
// Array case: use coerceRefArray for validation
var expectedLen = helpers.countDefiningCoords(shapeType, path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this want to be the count just for axLetter?

var pixelDflts = [0, 10];

// For each coordinate, coerce the position with their respective axis ref
[0, 1].forEach(function(i) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me like this whole loop should be skipped for paths - they don't want x0shift etc, and sizemode='pixel' doesn't make sense with an axis ref array. In fact seems like we should enforce that earlier on, and document it in the sizemode descriptions: if you have an axis reference array, sizemode can only be 'scaled'.

Comment on lines +154 to +157
if(ax.type === 'category' || ax.type === 'multicategory') {
coerce(axLetter + '0shift');
coerce(axLetter + '1shift');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise this shouldn't be done for paths

if(options.layer === 'above') {
drawShape(gd._fullLayout._shapeUpperLayer);
} else if(options.xref === 'paper' || options.yref === 'paper') {
} else if(options.xref.includes('paper') || options.yref.includes('paper')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clever, never knew you could call .includes on both arrays and strings!

// paper and axis domain referenced shapes don't affect autorange
if(shape.xref !== 'paper' && xRefType !== 'domain') {
if(xRefType === 'array') {
calcArrayRefAutorange(gd, shape, 'x');
Copy link
Contributor

@emilykl emilykl Jan 14, 2026

Choose a reason for hiding this comment

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

Is it possible to refactor calcArrayRefAutorange() as a pure function which doesn't modify the provided objects, but instead returns values which are then assigned/used in subsequent lines?

That would make it more transparent what is happening here, and would also make this if case more parallel with the following else if.

I realize it might not be possible without making this code pretty awkward, but could you consider it?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be more explicit, I'm picturing something along the lines of

const extremesForRefArray = calcArrayRefAutorange(gd, shape, 'x');
Object.entries(extremesForRefArray).forEach(([axID, axExtremes]) => {
  shape._extremes[ax._id] = Axes.findExtremes(ax, axExtremes.map(convertVal), paddingOpts);
});

}
};

function calcArrayRefAutorange(gd, shape, dim) {
Copy link
Contributor

Choose a reason for hiding this comment

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

call the third argument axLetter perhaps?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shapes that reference multiple axes simultaneously

4 participants