Image and shape clip paths #1453

alexcjohnson · 2017-03-08T06:11:26Z

In testing axis reference changes for integrating v1.24 into the workspace, I noticed some strange situations with clip paths for shapes and images. Turns out they had different issues but related symptoms. There's one particular edge case that I didn't sort out here because it's a much bigger structural question, so I described it in #1452 instead.

@etpinard please review.

particularly in cases of partial data referencing and layer: below

alexcjohnson · 2017-03-08T06:12:29Z

Note: annotations do not have these issues, as they don't use clip paths. We will happily allow the arrow and text box of an annotation to go outside the plot area as long as its reference point is visible.

alexcjohnson · 2017-03-08T06:13:14Z

src/components/images/draw.js

+        thisImage.call(Drawing.setClipUrl, clipAxes ?
+            ('clip' + fullLayout._uid + clipAxes) :
+            null
+        );


Needed to unset the clip path if you change from data-referenced to fully paper-referenced.

alexcjohnson · 2017-03-08T06:19:11Z

test/jasmine/tests/shapes_test.js

+
+    function getShape(index) {
+        var s = d3.selectAll('path[data-index="' + index + '"]');
+        expect(s.size()).toBe(1);


before the fix above, the shape was drawn twice in its initial configuration xref: 'x', yref: 'paper': once on subplot xy and again on xy2

Good eyes. Thanks very much!

etpinard · 2017-03-08T15:02:16Z

src/components/shapes/draw.js

+
+        path.call(Drawing.setClipUrl, clipAxes ?
+            ('clip' + gd._fullLayout._uid + clipAxes) :
+            null


Nice way to unset a clip path. 👍

etpinard · 2017-03-08T15:03:29Z

test/jasmine/tests/layout_images_test.js

@@ -425,23 +425,38 @@ describe('images log/linear axis changes', function() {
        // we don't try to figure out the position on a new axis / canvas
        // automatically when you change xref / yref, we leave it to the caller.

+        // initial clip path should end in 'xy' to match xref/yref
+        expect(d3.select('image').attr('clip-path') || '').toMatch(/xy\)$/);


Ha. I didn't know about .toMatch. Pretty handy 🔨

etpinard · 2017-03-08T15:04:55Z

test/jasmine/tests/layout_images_test.js

+            expect(d3.select('image').attr('clip-path') || '').toMatch(/x\)$/);
+
+            // change to full paper-referenced, to make sure the clip path disappears
+            return Plotly.relayout(gd, {'images[0].xref': 'paper'});


I'm surprised we weren't testing this before. Oh well, now it's 🔒 ed

etpinard · 2017-03-08T15:15:22Z

Looks like shapes_below_traces produced a diff

is that on purpose?

alexcjohnson · 2017-03-08T15:55:08Z

Looks like shapes_below_traces produced a diff, is that on purpose?

Hmm, maybe #1452 needs to be addressed together with this PR after all, that's what's going on here. You'll see that the data in the lower left subplot gets covered up in the new result for that image. But the original is clearly wrong in how it breaks the two shapes that span that subplot and either the one above or to the right into two disjoint pieces.

From a slack chat:

the reason I said (in #1452 ) "I'm not sure this really has a solution even theoretically” is that there really isn’t a layering that puts the shape in front of the plot area backgrounds but behind the traces, at least not when you consider inset subplots that want to have their own backgrounds in front of the main plot traces.

One easy thing we could do is put the below shapes completely below the plot layers… then you have to make plot_bgcolor transparent to see the shapes.

A trickier thing we could do is pull the background rects out of the subplot layers, putting them behind everything else, UNLESS we detect overlapping (but not overlaying) subplots.

That would cover all the use cases I can think of except below shapes that are half referenced to an axis of an inset subplot… which would be an exceedingly weird situation.

@etpinard suggested:

Maybe we should disallow shapes that span multiple subplots

That would certainly make this easier (the restriction would only apply to layer: 'below' shapes and images) - then we'd just have to figure out which subplot contains the shape. But there are some cool uses of subplot-spanning shapes - for example on the standard finance chart that has candlesticks/OHLC in the main plot and volume in a little lower subplot, if you want to highlight a time range. It would look weird to force these to be two separate shapes. Would also be annoying to try and enforce this restriction during editing (particularly via drag)

So I'm leaning toward the "trickier thing": pulling the plot backgrounds back to their own layer except for inset plots.

Thoughts?

alexcjohnson · 2017-03-08T15:58:42Z

#1390 is wrapped up in this too... better fix that at the same time.

etpinard · 2017-03-08T16:01:15Z

So I'm leaning toward the "trickier thing": pulling the plot backgrounds back to their own layer except for inset plots.

This is definitely the best solution from a user's perspective. If you feel like taking a shot at this, go for it 👍

alexcjohnson · 2017-03-08T23:16:30Z

2102b0f fixes #1390 and #1452 - I still want to add a couple of tests of the inset vs flat background behavior though

etpinard · 2017-03-09T16:40:09Z

src/components/images/draw.js

+
+        // filter out overlaid plots (which havd their images on the main plot)
+        // and gl2d plots (which don't support below images, at least not yet)
+        if(!subplotObj.imagelayer) continue;


Looks good 👍

etpinard · 2017-03-09T16:40:41Z

src/plot_api/plot_api.js

@@ -308,8 +308,7 @@ Plotly.plot = function(gd, data, layout, config) {

        // keep reference to shape layers in subplots
        var layerSubplot = fullLayout._paper.selectAll('.layer-subplot');
-        fullLayout._imageSubplotLayer = layerSubplot.selectAll('.imagelayer');
-        fullLayout._shapeSubplotLayer = layerSubplot.selectAll('.shapelayer');
+        fullLayout._shapeSubplotLayers = layerSubplot.selectAll('.shapelayer');


✏️ nicely done

etpinard · 2017-03-09T23:28:59Z

test/jasmine/tests/cartesian_test.js

+            expect(gd.querySelectorAll('.bg').length).toBe(behindCount + x2y2Count);
+        }
+
+        Plotly.plot(gd, [


Fantastic test 🎉

etpinard · 2017-03-09T23:30:03Z

💃 nicely done!

Don't forget to close off #1390 and #1452 after merging 🍻

alexcjohnson added 2 commits March 7, 2017 23:17

test and fix clip path removal on changing an image to full paper-ref

06523d6

test and fix correct clip path and count of drawn shapes

ae820ca

particularly in cases of partial data referencing and layer: below

alexcjohnson added status: reviewable bug labels Mar 8, 2017

alexcjohnson commented Mar 8, 2017

View reviewed changes

🔪 commented-out code

aeca8ae

alexcjohnson commented Mar 8, 2017

View reviewed changes

etpinard reviewed Mar 8, 2017

View reviewed changes

push subplot backgrounds to the way back unless they're inset plots

2102b0f

etpinard reviewed Mar 9, 2017

View reviewed changes

alexcjohnson added 2 commits March 9, 2017 17:09

test plot bg rect layering vs axis domains

10fe521

Merge branch 'master' into image-clip-path

521015a

etpinard reviewed Mar 9, 2017

View reviewed changes

alexcjohnson merged commit 8c23c67 into master Mar 9, 2017

alexcjohnson deleted the image-clip-path branch March 9, 2017 23:33

This was referenced Mar 9, 2017

Overdrawing of images (and probably shapes) #1390

Closed

Shapes layer: 'below' doesn't play well with subplots #1452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Image and shape clip paths #1453

Image and shape clip paths #1453

alexcjohnson commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

alexcjohnson Mar 8, 2017

alexcjohnson Mar 8, 2017

etpinard Mar 8, 2017

etpinard Mar 8, 2017

etpinard Mar 8, 2017

etpinard Mar 8, 2017

etpinard commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

etpinard commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

etpinard Mar 9, 2017

etpinard Mar 9, 2017

etpinard Mar 9, 2017

etpinard commented Mar 9, 2017

Image and shape clip paths #1453

Image and shape clip paths #1453

Conversation

alexcjohnson commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

etpinard commented Mar 8, 2017

alexcjohnson commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Mar 9, 2017