ContentPane: pre-processing callback for downloaded content

[Foam Head]

I'm working with a ContentPane that downloads its content via an href, but the content needs to be pre-processed before it is displayed. Despite having onLoad and onDownloadEnd callbacks, both of these are called *after* the content is displayed.

It looks like having onLoad fire after the content is displayed is identical in function to onDownloadEnd. I'm wondering if onLoad should be fired before the content is displayed. If onLoad and onDownloadEnd need to remain as is, then a new callback (onPreLoad? onDownloadPreprocess? preprocessDownload? preprocessContent?) should be added.

Note: My experience with this has been with a ContentPane, but the same pre-processing callback should be available in all classes that support downloading href content.

[Marcin Gołębski]

I vote for this change. In Dojo 1.1.0 the sequence of caling was as follows:

1. <fetching document from server>
2. onDownloadEnd()
3. <fetched document parsing>
4. onLoad()

and for me it was correct.

Now (DOJO 1.3.2 and up):

1. <fetching document from server>
2. <fetched document parsing>
3. onLoad()
4. onDownloadEnd()

so, onDownloadEnd is useless now, becouse is called just after onLoad.

I attached simple patch, whitch fixes this problem, is it correct approach?

[bill]

running out of time for 1.7

[dylan]

This oneliner looks like it should have been accepted and fixed 6 years ago. I'll review and land for 1.11, unless bill wants to handle it.

[bill]

It's unfortunate that Dojo 1.3 broke backwards compatibility with Dojo 1.1, but now this patch breaks backwards compatibility with Dojo 1.3 - 1.10, because existing code may assume the content is available (in this.containerNode) when onDownloadEnd() is called. So I don't think I want to change the current behavior of onDownloadEnd().

Having a new callback like preprocessContent(origContent) that returns the new content seems reasonable. You could call it from _setContent(). That means though that it would get called for inlined content too (from .set("content", ...) etc.), and sometimes the argument could be a DocumentFragment instead of a String.

On a related note, onLoad() is a little different than onDownloadEnd() because onLoad() is also called for .set("content", ...).

[dylan]

Bill, I've added ​https://github.com/dojo/dijit/pull/106

Please let me know if this is what you had in mind. I didn't add a test, as it's just a stub/placeholder, so doesn't really add any new functionality unless the user overrides it.

If you think this is better handled not as an event handler, but just as a callback, let me know and I'll change it.

[bill]

A few things:

1. It should have a test case, even though it's just a callback, in the same way that we already test onDownloadStart() and onDownloadEnd(). Specifically, Contentpane-remote.html tests those functions in the "onDownloadStart|End" test, so you should add some lines to that test to check that preprocessContent() is working too.

2. As I wrote in my previous comment, I meant for the function to return the modified content (or by default, the original content). After all, that's what this ticket is about: pre-processing the content before we set innerHTML. I guess that means that it's "just a callback" and should be named preprocessContent() rather than onPreprocessContent().

[dylan]

ok, that makes more sense. I'll have an updated PR in the next day or two. Thanks!

[dylan]

bill, I've created an updated PR. The tests may be too much in that each test is preprocessing the content, so we can scale that back if needed.

[Bill Keese <bill@…>]

In 5c2ae92/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled?