Fix issue 359 (zoom overlay drawn beneath shapes) #448

n-riesco · 2016-04-19T20:59:15Z

This PR comes from #395 . I couldn't rebase it, and I ended up cherry-picking the commits.

I followed @alexcjohnson 's advice and created a zoom layer between the info and the hover layers.

The PR is ready for review.

etpinard · 2016-04-19T21:38:57Z

test/jasmine/tests/select_test.js

+                .then(done);
+        });
+
+        it('should be appended to the the shape layer', function() {


you mean: to the zoom layer ?

etpinard · 2016-04-19T21:53:25Z

Visual confirmation (mock):

etpinard · 2016-04-19T21:56:54Z

test/jasmine/tests/select_test.js

+            expect(d3.selectAll('.zoomlayer > .zoombox-corners').size())
+                .toEqual(0);
+            expect(d3.selectAll('.zoomlayer > .select-outline').size())
+                .toEqual(2);


maybe add an assertion after doubleClick making sure that the select-outline are cleared.

etpinard · 2016-04-19T22:00:03Z

@n-riesco I made a few minor comments, but this PR looks great.

🍻 to our new shape expert.

n-riesco · 2016-04-20T10:38:44Z

click_test.js failed with:

Firefox 45.0.0 (Ubuntu 0.0.0) Test click interactions: drag interactions on ne dragbox should update the axis ranges FAILED
    Expected -3.011967491973726,2.1561305597186564 to be close to -3.01196749,1.7246647.
    require<["/home/user/github/plotly.js/test/jasmine/tests/click_test.js"]</</</</<@/tmp/c891e3533949b9cdd16cb0d3db6e00d8.browserify:100817:17 <- tests/click_test.js:222:0
    Expected -0.9910086301469277,1.389382716298284 to be close to -0.99100863,1.86546098.
    require<["/home/user/github/plotly.js/test/jasmine/tests/click_test.js"]</</</</<@/tmp/c891e3533949b9cdd16cb0d3db6e00d8.browserify:100818:17 <- tests/click_test.js:223:0
    Expected -3.011967491973726,2.1561305597186564 to be close to -3.01196749,2.08350047.
    require<["/home/user/github/plotly.js/test/jasmine/tests/click_test.js"]</</</</<@/tmp/c891e3533949b9cdd16cb0d3db6e00d8.browserify:100822:17 <- tests/click_test.js:227:0
    Expected -0.9910086301469277,1.389382716298284 to be close to -0.99100863,1.10938115.
    require<["/home/user/github/plotly.js/test/jasmine/tests/click_test.js"]</</</</<@/tmp/c891e3533949b9cdd16cb0d3db6e00d8.browserify:100823:17 <- tests/click_test.js:228:0

Skimming through the numbers made me think this was a case of a comparison with too many digits. I set the precision to 1e-6 and the tests passed. But now that I've checked the all numbers more carefully, I realised this test shouldn't have passed with a precision of 1e-6 (the y coordinate differed by up to 0.5).

This PR is not ready. I need to investigate further the reason for this random failures.

n-riesco · 2016-04-20T14:36:18Z

The drag interaction tests in click_test.js may fail (even in master) when run by
npm run test-jasmine while they succeed if run by npm run test-jasmine -- tests/click_test.js.
These failures disappear when a pause of 200ms is introduced in the beforeEach after creating the plot.
Shorter pauses of 0, 10, 20, 50 and 100 ms fail.

n-riesco · 2016-04-20T14:46:11Z

This PR is ready again for review/merging.

etpinard · 2016-04-20T14:48:47Z

test/jasmine/tests/click_test.js

@@ -180,7 +180,9 @@ describe('Test click interactions:', function() {

    describe('drag interactions', function() {
        beforeEach(function(done) {
-            Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done);
+            Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
+                setTimeout(done, 200);


I think #445 solves the same problem?

I thought Firefox wasn't affected by #445. I will adapt #445 to Firefox and see if the test works without the pause.

I couldn't adapt the solution to Firefox, because I couldn't find a flag similar to Chrome's --window-size.

Using @monfera 's trick I got to enable webgl in my VM's chromium, like this:

user@host:~/github/plotly.js$ git diff diff --git a/test/jasmine/karma.conf.js b/test/jasmine/karma.conf.js index 5905271..52e1ce4 100644 --- a/test/jasmine/karma.conf.js +++ b/test/jasmine/karma.conf.js @@ -69,7 +69,15 @@ func.defaultConfig = { // start these browsers // available browser launchers: https://npmjs.org/browse/keyword/karma-launcher - browsers: ['Chrome'], + browsers: ['Chrome_WindowSized'], + + // custom browser options + customLaunchers: { + Chrome_WindowSized: { + base: 'Chrome', + flags: ['--window-size=1035,617', '--enable-webgl', '--ignore-gpu-blacklist'] // values came from observing default size; all test cases pass with it + } + }, // Continuous Integration mode // if true, Karma captures browsers, runs the tests and exits

I ran the tests using Chromium and the drag-interaction tests work, but one of the gl tests failed (the one that used to fail with Firefox). See:

user@host:~/github/plotly.js$ npm run test-jasmine > [email protected] test-jasmine /home/user/github/plotly.js > karma start test/jasmine/karma.conf.js 20 04 2016 16:17:53.134:INFO [framework.browserify]: registering rebuild (autoWatch=true) 20 04 2016 16:18:02.923:INFO [framework.browserify]: 9104144 bytes written (8.99 seconds) 20 04 2016 16:18:02.988:INFO [framework.browserify]: bundle built 20 04 2016 16:18:03.009:WARN [karma]: No captured browser, open http://localhost:9876/ 20 04 2016 16:18:03.028:INFO [karma]: Karma v0.13.22 server started at http://localhost:9876/ 20 04 2016 16:18:03.034:INFO [launcher]: Starting browser Chrome 20 04 2016 16:18:05.306:INFO [Chromium 49.0.2623 (Ubuntu 0.0.0)]: Connected on socket /#f3LcZeKt1FWNckeOAAAA with id 18105360 Chromium 49.0.2623 (Ubuntu 0.0.0) Test gl plot interactions gl3d plots scatter3d click events should have FAILED TypeError: Cannot convert undefined or null to object at Object.<anonymous> (/tmp/b519d49625b3b7b09b7d4b9b755aee99.browserify:104577:31 <- tests/gl_plot_interact_test.js:141:0) LOG: 'lost context' LOG: 'lost context' LOG: 'lost context' LOG: 'lost context' LOG: 'restyle fail', undefined, '', undefined LOG: 'relayout fail', undefined, false LOG: 'type newtype already registered' LOG: 'Oops, tried to put a polar trace on an incompatible graph of cartesian data. Ignoring this dataset.', Object{type: 'newtype', r: 'this is polar'} Chromium 49.0.2623 (Ubuntu 0.0.0): Executed 773 of 773 (1 FAILED) (1 min 8.793 secs / 1 min 8.043 secs)

Shall I drop 04ef33e from this PR?

Shall I drop 04ef33e from this PR?

Yes please.

Those additional flags from Chrome would make a great addition, but they should be part of a different PR.

mdtusz · 2016-04-20T14:56:06Z

Looks great!

Fixes plotly#359

* Test whether select elements are appended to the zoom layer. * Test whether lasso elements are appended to the zoom layer.

* Ensured the zoombox corners have been removed before emitting 'plotly_deselect' or 'plotly_selected'.

* Test that a double click removes a selection.

* The drag interaction tests in click_test.js may fail when run by `npm run test-jasmine` while they succeed if run by `npm run test-jasmine -- tests/click_test.js`. * These failures disappear when a pause of 200ms is introduced in the `beforeEach` after creating the plot. * Shorter pauses of 0, 10, 20, 50 and 100 ms fail.

This reverts commit 9032cb7.

* In the drag interactions tests, make sure the notifier doesn't hide the drag elements.

n-riesco · 2016-04-27T12:50:33Z

This PR is ready again for review/merging.

etpinard · 2016-04-27T13:32:14Z

test/jasmine/tests/click_test.js

+                var tooltip = document.querySelector('.notifier-note');
+                if(tooltip) tooltip.style.display = 'None';
+
+                done();


Beautiful. Thanks for figuring this out 🍻

etpinard · 2016-04-27T13:33:37Z

@n-riesco Great PR. Thanks!

- a follow-up to #448

- broken since #448 - 🔒 down with test

n-riesco mentioned this pull request Apr 19, 2016

Fix zoom overlay positioning (Fixes #359) #395

Closed

etpinard added bug something broken status: reviewable labels Apr 19, 2016

etpinard reviewed Apr 19, 2016
View reviewed changes

n-riesco force-pushed the issue-359-add-zoomlayer branch from e2fa52f to 04ef33e Compare April 20, 2016 14:34

etpinard reviewed Apr 20, 2016
View reviewed changes

n-riesco force-pushed the issue-359-add-zoomlayer branch 2 times, most recently from cbee319 to 85c9948 Compare April 20, 2016 19:44

n-riesco added 8 commits April 26, 2016 15:51

Add zoomlayer

beccece

Fixes plotly#359

Add more tests to select_test.js

5df31d2

* Test whether select elements are appended to the zoom layer. * Test whether lasso elements are appended to the zoom layer.

Add zoom tests

2fa50c0

Fix test description in select_test.js

319a83b

Rename zoom_test.js to cartesian_test.js

78effe6

dragBox: remove corners before emitting an event

b7de50f

* Ensured the zoombox corners have been removed before emitting 'plotly_deselect' or 'plotly_selected'.

Update select_test.js

105bed9

* Test that a double click removes a selection.

Use comma with translate transforms

a360992

n-riesco force-pushed the issue-359-add-zoomlayer branch from 85c9948 to a360992 Compare April 26, 2016 14:52

n-riesco added 3 commits April 26, 2016 16:02

Revert "Pause before starting the drag-interaction tests"

03cad13

This reverts commit 9032cb7.

Fix failure in click_test.js

90700da

* In the drag interactions tests, make sure the notifier doesn't hide the drag elements.

etpinard reviewed Apr 27, 2016
View reviewed changes

etpinard merged commit b517896 into plotly:master Apr 27, 2016

n-riesco deleted the issue-359-add-zoomlayer branch April 27, 2016 16:37

etpinard added a commit that referenced this pull request Jun 28, 2017

rename variable plot to more appropriate zoomLayer

14ae77c

- a follow-up to #448

etpinard added a commit that referenced this pull request Jun 28, 2017

fix select/lasso on ternary subplots

4bbbe5c

- broken since #448 - 🔒 down with test

etpinard mentioned this pull request Jun 28, 2017

Fix scatterternary lasso/select drag modes #1831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 359 (zoom overlay drawn beneath shapes) #448

Fix issue 359 (zoom overlay drawn beneath shapes) #448

n-riesco commented Apr 19, 2016

etpinard Apr 19, 2016

etpinard commented Apr 19, 2016

etpinard Apr 19, 2016

etpinard commented Apr 19, 2016

n-riesco commented Apr 20, 2016

n-riesco commented Apr 20, 2016

n-riesco commented Apr 20, 2016

etpinard Apr 20, 2016

n-riesco Apr 20, 2016

n-riesco Apr 20, 2016

etpinard Apr 20, 2016

mdtusz commented Apr 20, 2016

n-riesco commented Apr 27, 2016

etpinard Apr 27, 2016

etpinard commented Apr 27, 2016

Fix issue 359 (zoom overlay drawn beneath shapes) #448

Fix issue 359 (zoom overlay drawn beneath shapes) #448

Conversation

n-riesco commented Apr 19, 2016

etpinard Apr 19, 2016

Choose a reason for hiding this comment

etpinard commented Apr 19, 2016

etpinard Apr 19, 2016

Choose a reason for hiding this comment

etpinard commented Apr 19, 2016

n-riesco commented Apr 20, 2016

n-riesco commented Apr 20, 2016

n-riesco commented Apr 20, 2016

etpinard Apr 20, 2016

Choose a reason for hiding this comment

n-riesco Apr 20, 2016

Choose a reason for hiding this comment

n-riesco Apr 20, 2016

Choose a reason for hiding this comment

etpinard Apr 20, 2016

Choose a reason for hiding this comment

mdtusz commented Apr 20, 2016

n-riesco commented Apr 27, 2016

etpinard Apr 27, 2016

Choose a reason for hiding this comment

etpinard commented Apr 27, 2016