Last Comment Bug 723431 - DOMTemplate should allow customisation of display of null/undefined values
: DOMTemplate should allow customisation of display of null/undefined values
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks: 736831
  Show dependency treegraph
 
Reported: 2012-02-02 01:12 PST by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-05-04 07:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (10.63 KB, patch)
2012-02-07 04:33 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
Details | Diff | Review
upload 2 (10.20 KB, patch)
2012-02-13 14:47 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
Details | Diff | Review
upload 3 (10.71 KB, patch)
2012-04-03 09:45 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-02 01:12:11 PST
Currently:

    <div id=a>${foo}</div>
        +
    domtemplate('a', { foo: null });
        V
    <div id=a>null</div>

This is good for debugging, but it's annoying - frequently you want null/undefined to show up as an empty string.

We should add an option so:

    <div id=a>${foo}</div>
        +
    domtemplate('a', { foo: null }, { blankNullUndefined: true });
        V
    <div id=a></div>
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-07 04:26:32 PST
https://tbpl.mozilla.org/?tree=Try&rev=b9d6b2377f5a
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-07 04:33:26 PST
Created attachment 594990 [details] [diff] [review]
upload 1
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-08 11:52:31 PST
If you'd like to see the extra parts cut-up github style: https://github.com/campd/gcli/pull/14
Comment 4 Dave Camp (:dcamp) 2012-02-10 10:18:39 PST
Comment on attachment 594990 [details] [diff] [review]
upload 1

Review of attachment 594990 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/Templater.jsm
@@ +54,5 @@
>   * @param node A DOM element or string referring to an element's id
>   * @param data Data to use in filling out the template
>   * @param options Options to customize the template processing. One of:
>   * - allowEval: boolean (default false) Basic template interpolations are
> + *   either property paths (e.g. ${a.b.c.d}), however if allowEval=true then we

This isn't new, but that 'either' seems to be missing an 'or'.
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-13 14:47:51 PST
Created attachment 596797 [details] [diff] [review]
upload 2

<blush>

I forgot to do the null/undefined processing on attribute values. See here for the fix:

           } else {
             // Replace references in all other attributes
             var newValue = value.replace(this._templateRegion, function(path) {
-              return this._envEval(path.slice(2, -1), data, value);
+              var insert = this._envEval(path.slice(2, -1), data, value);
+              if (this.options.blankNullUndefined && insert == null) {
+                insert = '';
+              }
+              return insert;
             }.bind(this));
             // Remove '_' prefix of attribute names so the DOM won't try
             // to use them before we've processed the template

Also this fixes your note about the either/or in the doc-comment, and adds tests for the above change.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-15 00:16:21 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=723431
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-03 09:45:34 PDT
Created attachment 611850 [details] [diff] [review]
upload 3

You reviewed this ages ago, but I never committed.

I re-pushed to try (https://tbpl.mozilla.org/?tree=Try&pusher=jwalker@mozilla.com) and rolled in the one char fix to bug 736831, which you reviewed yesterday:
https://github.com/joewalker/gcli/commit/d4d93245aff9ad3cdd81733696443bd8a416844d and 
https://github.com/campd/gcli/pull/27#issuecomment-4883212

It seems like a no-brainer that I can commit this, but I'm just checking.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-04-25 05:57:58 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9d827d67bfbb
Comment 9 Tim Taubert [:ttaubert] 2012-04-27 05:48:26 PDT
https://hg.mozilla.org/mozilla-central/rev/be802d70aa5f
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 13:24:42 PDT
This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-05-03 07:14:40 PDT
https://hg.mozilla.org/integration/fx-team/rev/d1195d800fde
Comment 12 Tim Taubert [:ttaubert] 2012-05-04 07:30:18 PDT
https://hg.mozilla.org/mozilla-central/rev/d1195d800fde

Note You need to log in before you can comment on or make changes to this bug.