Opened 10 years ago

Closed 4 years ago

#9622 closed enhancement (fixed)

ContentPane: pre-processing callback for downloaded content

Reported by: Foam Head Owned by: bill
Priority: high Milestone: 1.11
Component: Dijit Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

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.

Attachments (1)

ContentPane.patch (613 bytes) - added by Marcin Gołębski 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by bill

Milestone: tbd1.5

comment:2 Changed 10 years ago by 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?

Changed 10 years ago by Marcin Gołębski

Attachment: ContentPane.patch added

comment:3 Changed 9 years ago by Adam Peller

Milestone: 1.51.6

comment:4 Changed 9 years ago by bill

Description: modified (diff)
Milestone: 1.61.7
Summary: RFE: pre-processing callback for downloaded contentContentPane: pre-processing callback for downloaded content

comment:5 Changed 8 years ago by bill

Milestone: 1.71.8

running out of time for 1.7

comment:6 Changed 7 years ago by bill

Milestone: 1.8future

comment:7 Changed 4 years ago by dylan

Milestone: future1.11
Owner: set to dylan
Status: newassigned

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.

comment:8 Changed 4 years ago by 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", ...).

comment:9 Changed 4 years ago by dylan

Owner: changed from dylan to bill

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.

comment:10 Changed 4 years ago by 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.
  1. 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().

comment:11 Changed 4 years ago by dylan

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

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: assignedclosed

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? 
Note: See TracTickets for help on using tickets.