fix IE Drag & Drop Bugs

fix IE Drag & Drop Bugs

Refactored part of the drag and drop system to
allow users on IE to drop the charms on the 
"Drop here" help text in the center of the canvas.

https://code.launchpad.net/~hatch/juju-gui/ie-dd-fix/+merge/179298

(do not edit description out of merge proposal)

[jeff.pihach, 2013-08-08 22:44:15]

Please take a look.

[huwshimi, 2013-08-09 01:47:34]

On 2013/08/08 22:44:15, jeff.pihach wrote:
> Please take a look.

This QAs OK for me in all three browsers.

[gary.poster, 2013-08-09 06:25:59]

LGTM.  Did not qa (Don't have IE locally, and Huw did it anyway :-P).  I have a
few small questions/suggestions, and a longer term concern for discussion.

Thanks

Gary

https://codereview.appspot.com/12683044/diff/1/app/assets/javascripts/d3-comp...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/12683044/diff/1/app/assets/javascripts/d3-comp...
app/assets/javascripts/d3-components.js:463: this._rendered = true;
Out of pure curiosity, am I right in assuming that this change is a fly-by style
change rather than actually important in fixing the IE issue?

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:89: }
Since the idea of this section is that it is supposed to be a place readers can
quickly read what all of the event handlers are, I'd recommend making a comment
pointing the reader to _attachDragEvents.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:188: events to stop bubbling at odd places cross
browser.
This is a concerning characteristic of the topology event handler registration
code.  Have you and Ben identified any underlying causes and longer term
resolutions?  Does at least one underlying layer have to do with YUI events
being better cross-browser than d3 events?  Looking at your changes, that seems
to be pertinent.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:213: e.stopPropagation();
Aren't those two equivalent to e.halt()?  If so, let's make the minor
simplification.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:428: this._ignore(e);
If I'm right that _ignore should just call halt, let's just call halt here
rather than using _ignore.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:436: var dropXY = [evt.clientX, (evt.clientY -
71)];
Please add a comment explaining what 71 is. :-)

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:731: // times this module is rendered.
I'm assuming that the normal topology event machinery handles this for you?

[jeff.pihach, 2013-08-09 16:42:45]

Thanks for the review! Changes coming

https://codereview.appspot.com/12683044/diff/1/app/assets/javascripts/d3-comp...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/12683044/diff/1/app/assets/javascripts/d3-comp...
app/assets/javascripts/d3-components.js:463: this._rendered = true;
Right just a flyby - I had added some code here but since removed it. I left
this change because it made a little more sense to read.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:89: }
On 2013/08/09 06:25:59, gary.poster wrote:
> Since the idea of this section is that it is supposed to be a place readers
can
> quickly read what all of the event handlers are, I'd recommend making a
comment
> pointing the reader to _attachDragEvents.

Done.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:188: events to stop bubbling at odd places cross
browser.
I spent a bit of time tracking it down but was still far from the cause and
subsequent fix so I had to abandon the search to stay within the timebox. I'll
create a ticket to track down the root cause as a slack.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:213: e.stopPropagation();
On 2013/08/09 06:25:59, gary.poster wrote:
> Aren't those two equivalent to e.halt()?  If so, let's make the minor
> simplification.

Done.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:428: this._ignore(e);
On 2013/08/09 06:25:59, gary.poster wrote:
> If I'm right that _ignore should just call halt, let's just call halt here
> rather than using _ignore.

Done.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:436: var dropXY = [evt.clientX, (evt.clientY -
71)];
Oops, done!

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:731: // times this module is rendered.
I originally had a method in the topology calling one here during it's "render
once" cycle but it was confusing when I had others review so a decision was made
to move the check here to contain the functionality.

[benjamin.saller, 2013-08-09 17:16:07]

LGTM w/o full QA as that was done

Thanks for tracking this down

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:188: events to stop bubbling at odd places cross
browser.
On 2013/08/09 16:42:45, jeff.pihach wrote:
> I spent a bit of time tracking it down but was still far from the cause and
> subsequent fix so I had to abandon the search to stay within the timebox. I'll
> create a ticket to track down the root cause as a slack.

You don't think this is related to the special case handling in the browser
around drag/drop? To handle those events yourself you are supposed to return
false from the handler iirc from the spec. I think its doing something strange
specially here.

https://codereview.appspot.com/12683044/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:731: // times this module is rendered.
On 2013/08/09 06:25:59, gary.poster wrote:
> I'm assuming that the normal topology event machinery handles this for you?

There isn't special support for 'once' styled events, but we talked about this
and it made sense here.

[jeff.pihach, 2013-08-09 18:06:27]

*** Submitted:

fix IE Drag & Drop Bugs

Refactored part of the drag and drop system to
allow users on IE to drop the charms on the 
"Drop here" help text in the center of the canvas.

R=huwshimi, gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/12683044