Opened 11 years ago

Last modified 3 years ago

#9263 assigned defect

dijit.layout.ContentPane doesn't allow replacing invalid content when setter fails

Reported by: Phil DeJarnett Owned by: dylan
Priority: low Milestone: 1.15
Component: Dijit Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

Currently, the _onError(type, err, consoleText) method of dijit.layout.ContentPane? will never show the message returned from onContentError due to the way it is written. It current looks like this:

var errText = this['on' + type + 'Error'].call(this, err);
if(consoleText){
	console.error(consoleText, err);
}else if(errText){// a empty string won't change current content
	this._setContent(errText, true);
}

The problem is that when there is, for example, a requires error, the html setter always returns consoleText.

Instead, I think the else should be removed, and the functions should look like this:

if(consoleText){
	console.error(consoleText, err);
}
var errText = this['on' + type + 'Error'].call(this, err);
if(errText){// a empty string won't change current content
	this._setContent(errText, true);
}

This will allow for more robust error handling.

Change History (12)

comment:1 Changed 11 years ago by bill

Owner: set to Sam Foster

Sam, any idea on this one? The code in question is in both ContentPane.js and dojo/html.js, but I have no idea why.

comment:2 Changed 11 years ago by bill

I traced the code down to [9399], and really from before then, where it was using console.error() to report errors from the parser.

I don't see a good reason for it though, it seems like (as OverZealous said) we could get rid of it.

comment:3 Changed 10 years ago by Foam Head

FWIW, I'm working on something that needs onContentError now. The fix OverZealous? proposed looks correct to me. Any chance of seeing this in a 1.3 dot release?

comment:4 Changed 10 years ago by Sam Foster

The duplication between html.js and ContentPane.js's onError and onContentError exists to preserve existing extension points for ContentPane. Its not ideal and needs resolving if that's possible without breaking anyone.

ContentPane provides an overide for the _onError setting method, so all errors going through this dispatcher are handled by the ContentPane instead of the setter. So bug #1 is that _ContentSetter bypasses this dispatcher by calling its this.onContentError directly.

ContentPane's _onError method apes _ContentSetter's onError in calling the specific error method to get an error message string to replace the node content with. But ContentPane's onContentError method is just a stub, and returns nothing out of the box. As a result, errText is empty unless that method has been extended. AFAICT consoleText is never passed? And errText will not typically have a value so the outcome is that nothing happens.

This is just from tracing through the code, so apologies if I'm adding to the confusion by misunderstanding something. What would be enormously helpful here would be some unit tests to properly exercise the error handling.

comment:5 Changed 10 years ago by Foam Head

Actually, per the test case I attached to #9611, something is causing the return string of onContentError to not be displayed. While #9611 may include unsupported ways to override onContentError, *none* of them ever show the return string in the content. Since you get a console error for every test case, I assumed this bug was the culprit. (Note: #9611 is about which methods are supported to override on*Error functions and my belief that at least one that should be supported isn't working -- which is different from this issue.)

While I didn't fully follow your detailed explanation, this bug is not necessarily about redesigning the html.js and ContentPane?.js _onError/on*Error interactions. It is primarily a request to have onContentError's return string properly set as the document's content. OverZealous?' analysis and proposed fix are perfect for this.

So can I request that, assuming you agree, you implement OverZealous?' fix for both html.js's and ContentPane?.js's _onError for a 1.3 point release and open a second ticket to track the larger issue of _onError/on*Error interactions for a 1.4 or later release?

comment:6 Changed 10 years ago by bill

Milestone: tbd1.5

comment:7 Changed 9 years ago by Sam Foster

Milestone: 1.51.6

comment:8 Changed 9 years ago by bill

Milestone: 1.61.7

comment:9 Changed 8 years ago by Sam Foster

Milestone: 1.7future

Sorry I have to punt this again. Fixing ContentPane? bugs is a game of whackamole and any fix needs enough time to bake properly before release to be sure it's not making the situation worse for someone. For the last few releases I've just not had time during that window - though as it turned out 1.7 went way long. If you do, please step up.

comment:10 Changed 7 years ago by bill

Priority: highlow

comment:11 Changed 4 years ago by dylan

Milestone: future1.12
Owner: changed from Sam Foster to dylan
Status: newassigned

comment:12 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.