Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16430 closed task (fixed)

Tree: exceptions to console when expand/collapse animation interrupted

Reported by: jonferraiolo Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

See discussion in this Maqetta issue: https://github.com/maqetta/maqetta/issues/3519

The Maqetta team believes that dijit.Tree is not handling deferred properly.

Change History (8)

comment:1 Changed 7 years ago by Adam Peller

Component: GeneralDijit
Owner: set to bill

comment:2 Changed 7 years ago by bill

Milestone: tbd1.9
Status: newassigned
Summary: dijit.Tree not handling deferred properly?Tree: exceptions to console when expand/collapse animation interrupted
Type: defecttask

This error, or a similar one, can be reproduced by quickly expanding and collapsing a TreeNode (for example, the root node in the first tree in test_Tree.html).

Tree uses Deferred's to keep track of the expand/collapse animation of TreeNodes. The animations are cancelled (like in the above case) by canceling the Deferred. Canceling a Deferred counts as rejecting it, and by design, in debug mode, whenever a Deferred is rejected, and the rejection is not handled, there's a log message to the console.

The Tree code internally has some cases where it is not catching rejections on the Deferred objects returned from TreeNode.expand() and TreeNode.collapse().

In your case, you are calling selectPath() which ends up calling TreeNode.expand() which calls this._collapseDeferred.cancel(). this._collapseDeferred is the Deferred returned from collapse(). It's silly because the collapse animation has presumably already finished, but anyway that's enough to trigger the errors.

comment:3 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30124]:

Refactor Tree code using Deferreds, leveraging chaining abilities of new dojo/Deferred module that allow expressions like promise1.then(function(){ return promise2; }).then(function(){ return promise3; }), which don't resolve until promise1, promise2, and promise3 have all resolved.

This indirectly fixes spurious console error messages when a Tree animation is cancelled, because for example the user started expanding a node but then clicked again to collapse it, before the original animation was finished. The problem was that there was previously code like animationDeferred.then(successHandler), with no failureHandler, and that caused dojo/promise/instrumentation to print the log messages.

Fixes #16430 !strict.

comment:4 Changed 7 years ago by bill

In [30125]:

Return Promise rather than Deferred from public methods, but as usual shimmed with the addCallback() and addErrback() methods for backwards compatibility.

Refs #16430 !strict.

comment:5 Changed 7 years ago by ben hockey

did you check that the shimming in r30125 actually works? as far as i know, the promise is frozen (so you'll need to check this in a browser that supports Object.freeze) and you can't add methods to it. the way to work around it is to use lang.delegate and then add the properties to the delegated object.

i don't know that what you've done won't work but it looks like it may not.

comment:6 Changed 7 years ago by bill

I tried in Chrome, with no exceptions being thrown, but on further testing it appears to just fail silently. I guess I'll try adding a lang.delegate() although not sure if that works either, as it creates a new this pointer.

comment:7 Changed 7 years ago by bill

PS: delegate() seems to work fine.

require(["dojo/Deferred", "dojo/_base/lang"],
 function(Deferred, lang){
   var d = new Deferred();
   var p = d.promise;
   var pd = lang.delegate(p, {
      addCallback: function(cb){ p.then(cb); },
      addErrback: function(eb){ p.otherwise(eb); }
   });
   pd.then(function(val){ 
      console.log("then returns ", val);
   }, function(err){
      console.log("then exception ", err);
   });
   pd.addCallback(function(val){ 
      console.log("addCallback returns ", val);
   });
   pd.addErrback(function(err){ 
      console.log("addErrback returns ", err);
   });
   d.resolve(43);
   //d.reject(new Error("testing"));
});

comment:8 Changed 7 years ago by bill

In [30131]:

Need to use lang.delegate() to add methods to Promise because it might be frozen, thanks neonstalwart. Refs #16430 !strict.

Note: See TracTickets for help on using tickets.