XHTML-capable DOM utilities.

And, have gadgets.rpc make use of it.

[bsittler, 2011-05-07 00:43:28]

LGTM

Small nits only:

http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/core.util.dom/dom.js (right):

http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/core.util.dom/dom.js:29: gadgets.util =
function() {
Please update to not stomp an existing gadgets.util definition. Remember, some
people use Shindig without fancy JS compilers :)

http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/core.util.dom/dom.js:48: arr.push('
').push(attrib).push('="').push(attribs[attrib]).push('"');
Please quote attribute values as they may contain syntactically significant
characters (" or &, most importantly.) At a minimum:

arr.push(' ').push(attrib).push('="').
   push((attribs[attrib]).split('&').join('&').split('"').join('"')).
   push('"');

Defense-in-depth would also convert \uff02 to ＂ and \uff06 to ＆ as
some configurations of Internet Explorer count those as syntactically
significant (and equivalent to " and &, respectively!) too:


arr.push(' ').push(attrib).push('="').
   push((attribs[attrib]).
    split('&').join('&').
    split('"').join('"').
    split('\uff02').join('＂').
    split('\uff06').join('＆')).
   push('"');

Too bad you can't mutually depend on gadgets.util.escapeString. Maybe it should
live here too, and you can leave a pointer in gadgets.util? While you're at it,
you could fix its handling of UTF-16 surrogate pairs too, but I understand if
you decide that's too far out-of-scope.

[mhermanto, 2011-05-07 02:31:56]

On Fri, May 6, 2011 at 5:43 PM, <bsittler@google.com> wrote:

> LGTM
>
> Small nits only:
>

Glad to hear that this API will work for our use-case.


>
>
http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
> File features/src/main/javascript/features/core.util.dom/dom.js (right):
>
>
>
http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/core.util.dom/dom.js:29:
> gadgets.util = function() {
> Please update to not stomp an existing gadgets.util definition.
>

The stomping is indeed an issue, largely due to (at the time) need to
isolate gadgets.util.urlparams.
What makes it weird here is that different features share a namespace. I
tried fixing this by having separate features (core.util.dom,
core.util.string), only to be blocked by unit tests not happy with my
refactoring. The previous uploaded changelist reflect this. So, to unblock,
I'm going to piggy-back on core.util with TODO.


> Remember, some people use Shindig without fancy JS compilers :)
>

Yes, this is the tricky part.


>
>
http://codereview.appspot.com/4506043/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/core.util.dom/dom.js:48:
> arr.push(' ').push(attrib).push('="').push(attribs[attrib]).push('"');
> Please quote attribute values as they may contain syntactically
> significant characters (" or &, most importantly.) At a minimum:
>
> arr.push(' ').push(attrib).push('="').
>
> push((attribs[attrib]).split('&').join('&amp;').split('"').join('&quot;')).
>   push('"');
>
> Defense-in-depth would also convert \uff02 to &#65282; and \uff06 to
> &#65286; as some configurations of Internet Explorer count those as
> syntactically significant (and equivalent to " and &, respectively!)
> too:
>
>
> arr.push(' ').push(attrib).push('="').
>   push((attribs[attrib]).
>    split('&').join('&amp;').
>    split('"').join('&quot;').
>    split('\uff02').join('&#65282;').
>    split('\uff06').join('&#65286;')).
>   push('"');
>
> Too bad you can't mutually depend on gadgets.util.escapeString. Maybe it
> should live here too, and you can leave a pointer in gadgets.util? While
> you're at it, you could fix its handling of UTF-16 surrogate pairs too,
> but I understand if you decide that's too far out-of-scope.


I've piggybacked on core.util so gadgets.util.escapeString() is available.
Wrt UTF-16, I believe escapeString() covers this.

Patch updated. PTAL.


>
>
> http://codereview.appspot.com/4506043/
>

[mhermanto, 2011-05-07 02:32:19]

Addressing Ben's comments. Thanks!

[bsittler, 2011-05-07 05:00:14]

LGTM

Only a couple more minor nits:

http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
File features/src/main/javascript/features/core.util/util.js (right):

http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
features/src/main/javascript/features/core.util/util.js:108: ch =
str.charCodeAt(i);
This function sort of handles UTF-16 by not escaping it, but fails to address
wide-character markup characters (the wide characters in the range U+FF01 …
U+FF5E correspond to the narrow characters in the range U+0021 … U+007E in some
configurations of Internet Explorer, and are interpreted as markup equivalent to
their narrow counterparts in some contexts.) Please fix. Minimal fix would be to
add the following to escapeCodePoints:
  • 0xff02, a.k.a 65282 — fullwidth quotation mark
  • 0xff07, a.k.a 65287 — fullwidth apostrophe
  • 0xff1c, a.k.a 65308 — fullwidth less-than sign
  • 0xff1e, a.k.a 65310 — fullwidth greater-than sign
  • 0xff3c, a.k.a 65340 — fullwidth reverse solidus

Also FWIW unescapeEntity is b0rked for UTF-16 and can easily be convinced to
generate truncating NULs or completely invalid non-Unicode characters, but you
didn't touch any of it but the trailing "}" so don't feel obligated to fix in
this change if you don't want to. On the other hand, here's a fixed version if
you want to drop it in (it handles entities for valid codepoints from U+0001 …
U+10FFFD, except for the non-character codepoints U+…FFFE and U+…FFFF; isolated
UTF-16 surrogate pairs are supported for compatibility with previous versions of
escapeString, 0 generates the empty string rather than a possibly-truncating
'\0', and all other inputs generate U+FFFD (the replacement character, standard
practice for non-signalling Unicode codecs like this one):

function unescapeEntity(match, value) {
  return (
      (value > 0) &&
      (value <= 0x10fffd) &&
      ((value & 0xffff) < 0xfffe)) ?
    ((value <= 0xffff) ?
      String.fromCharCode(value) :
      String.fromCharCode(
        ((value - 0x10000) >> 10) | 0xd800,
        ((value - 0x10000) & 0x3ff) | 0xdc00)) :
    ((value === 0) ? '' : '\ufffd');
}

http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
features/src/main/javascript/features/core.util/util.js:292: * a bunch of
regular expressions.
I believe this TODO is obsolete (and has been for a while.) Please remove it.

http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
features/src/main/javascript/features/core.util/util.js:366: * @param {Object=}
opt_attribs Optional set of attributes to attach.
You might want to mention that the only attributes which will work here are ones
which are spelled the same way in XHTML attribute naming (generally the most
strict, and all-lower-case), HTML attribute naming (less strict, and
case-insensitive), and JavaScript property naming (some properties named
incompatibly with XHTML/HTML). Perhaps also a TODO is in order to provide
automatic mapping, or to only set the needed and JS-HTML-XHTML compatible subset
through stringifyElement (just 'name' and 'id', AFAIK) Also note that the values
of the attributes will be stringified should the stringifyElement code path be
taken (IE)

[mhermanto, 2011-05-09 19:53:20]

Addressing more Ben's comments.

[mhermanto, 2011-05-09 19:53:56]

On Fri, May 6, 2011 at 10:00 PM, <bsittler@google.com> wrote:

> LGTM
>
> Only a couple more minor nits:
>
>
>
>
http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
> File features/src/main/javascript/features/core.util/util.js (right):
>
>
>
http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
> features/src/main/javascript/features/core.util/util.js:108: ch =
> str.charCodeAt(i);
> This function sort of handles UTF-16 by not escaping it, but fails to
> address wide-character markup characters (the wide characters in the
> range U+FF01 … U+FF5E correspond to the narrow characters in the range
> U+0021 … U+007E in some configurations of Internet Explorer, and are
> interpreted as markup equivalent to their narrow counterparts in some
> contexts.) Please fix. Minimal fix would be to add the following to
> escapeCodePoints:
>  • 0xff02, a.k.a 65282 — fullwidth quotation mark
>  • 0xff07, a.k.a 65287 — fullwidth apostrophe
>  • 0xff1c, a.k.a 65308 — fullwidth less-than sign
>  • 0xff1e, a.k.a 65310 — fullwidth greater-than sign
>  • 0xff3c, a.k.a 65340 — fullwidth reverse solidus
>
>
Thanks for the explanation. Updated escapePoints.


> Also FWIW unescapeEntity is b0rked for UTF-16 and can easily be
> convinced to generate truncating NULs or completely invalid non-Unicode
> characters, but you didn't touch any of it but the trailing "}" so don't
> feel obligated to fix in this change if you don't want to.




> On the other
> hand, here's a fixed version if you want to drop it in (it handles
> entities for valid codepoints from U+0001 … U+10FFFD, except for the
> non-character codepoints U+…FFFE and U+…FFFF; isolated UTF-16 surrogate
> pairs are supported for compatibility with previous versions of
> escapeString, 0 generates the empty string rather than a
> possibly-truncating '\0', and all other inputs generate U+FFFD (the
> replacement character, standard practice for non-signalling Unicode
> codecs like this one):
>
> function unescapeEntity(match, value) {
>  return (
>      (value > 0) &&
>      (value <= 0x10fffd) &&
>      ((value & 0xffff) < 0xfffe)) ?
>    ((value <= 0xffff) ?
>      String.fromCharCode(value) :
>      String.fromCharCode(
>        ((value - 0x10000) >> 10) | 0xd800,
>        ((value - 0x10000) & 0x3ff) | 0xdc00)) :
>    ((value === 0) ? '' : '\ufffd');
> }
>

Thanks for the thorough explanation Ben, I will include this in
another/separate CL.


>
>
>
http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
> features/src/main/javascript/features/core.util/util.js:292: * a bunch
> of regular expressions.
> I believe this TODO is obsolete (and has been for a while.) Please
> remove it.
>

Done.

>
>
>
http://codereview.appspot.com/4506043/diff/9002/features/src/main/javascript/...
> features/src/main/javascript/features/core.util/util.js:366: * @param
> {Object=} opt_attribs Optional set of attributes to attach.
> You might want to mention that the only attributes which will work here
> are ones which are spelled the same way in XHTML attribute naming
> (generally the most strict, and all-lower-case), HTML attribute naming
> (less strict, and case-insensitive), and JavaScript property naming
> (some properties named incompatibly with XHTML/HTML). Perhaps also a
> TODO is in order to provide automatic mapping, or to only set the needed
> and JS-HTML-XHTML compatible subset through stringifyElement (just
> 'name' and 'id', AFAIK) Also note that the values of the attributes will
> be stringified should the stringifyElement code path be taken (IE)
>

JS doc updated. Added TODO.


>
> http://codereview.appspot.com/4506043/
>

[mhermanto, 2011-05-09 19:54:32]

John's are you ok with gadgets.rpc to use the new iframe creation?

On Mon, May 9, 2011 at 12:53 PM, <mhermanto@gmail.com> wrote:

> Addressing more Ben's comments.
>
>
> http://codereview.appspot.com/4506043/
>

[johnfargo, 2011-05-09 19:59:15]

I'd prefer to see these new utils put into a core.dom package for dependency
management purposes.

I don't see a corresponding g.rpc change in this CL. Am I missing something?

On 2011/05/09 19:54:32, mhermanto wrote:
> John's are you ok with gadgets.rpc to use the new iframe creation?
> 
> On Mon, May 9, 2011 at 12:53 PM, <mailto:mhermanto@gmail.com> wrote:
> 
> > Addressing more Ben's comments.
> >
> >
> > http://codereview.appspot.com/4506043/
> >

[mhermanto, 2011-05-09 20:05:01]

On Mon, May 9, 2011 at 12:59 PM, <johnfargo@gmail.com> wrote:

> I'd prefer to see these new utils put into a core.dom package for
> dependency management purposes.
>

Me too, but as tried on Friday, I couldn't find out why tests keep barfing.
So, I'd rather get this in now. Has been TODO'ed.

>
> I don't see a corresponding g.rpc change in this CL. Am I missing
> something?


Er, you're right. Changelist updated.


> On 2011/05/09 19:54:32, mhermanto wrote:
>
>> John's are you ok with gadgets.rpc to use the new iframe creation?
>>
>
>  On Mon, May 9, 2011 at 12:53 PM, <mailto:mhermanto@gmail.com> wrote:
>>
>
>  > Addressing more Ben's comments.
>> >
>> >
>> > http://codereview.appspot.com/4506043/
>> >
>>
>
>
>
> http://codereview.appspot.com/4506043/
>

[bsittler, 2011-05-09 20:09:14]

LGTM

Consider adding a TODO for unescapeEntity, but submit either way.

[mhermanto, 2011-05-09 21:18:02]

On Mon, May 9, 2011 at 1:04 PM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Mon, May 9, 2011 at 12:59 PM, <johnfargo@gmail.com> wrote:
>
>> I'd prefer to see these new utils put into a core.dom package for
>> dependency management purposes.
>>
>
> Me too, but as tried on Friday, I couldn't find out why tests keep barfing.
> So, I'd rather get this in now. Has been TODO'ed.
>

This CL has been submitted as is (less rpc change, but still want your
comment on it).
Found out why tests fail, next CL will include core.dom|string refactoring.


>> I don't see a corresponding g.rpc change in this CL. Am I missing
>> something?
>
>
> Er, you're right. Changelist updated.
>
>
>>  On 2011/05/09 19:54:32, mhermanto wrote:
>>
>>> John's are you ok with gadgets.rpc to use the new iframe creation?
>>>
>>
>>  On Mon, May 9, 2011 at 12:53 PM, <mailto:mhermanto@gmail.com> wrote:
>>>
>>
>>  > Addressing more Ben's comments.
>>> >
>>> >
>>> > http://codereview.appspot.com/4506043/
>>> >
>>>
>>
>>
>>
>> http://codereview.appspot.com/4506043/
>>
>
>