Hm...
I guess I was fooled because I didn't now about getTypeForName!
It seems odd to me that we would have 3 different names for the same type. I
am probably not the only one who would think that these 3 different types might
have a purpose.
I was also fooled into thinking the type "string" should mean ECMAScript
"String" because a) ScriptCompiler.quote is used to encode it, and b) we
document that equivalence in the doc (don't we)?
Is there documentation on what "text" and "html" types are supposed to mean?
(My intuition is that "text" should mean "interpreted as CDATA" and "html"
should mean "interpreted as node 'content'". I.e., < and & don't have any
special meaning in the former, but they do in the latter.
Perhaps we need to take a poll on laszlo-user to find out what the
least-breaking change would be, i.e., what most people believe?
On 2010-05-25, at 16:31, André Bargull wrote:
> Let's give it a new try.
>
> type="text" and type="html" are handled like type="string" (minus some
> special code for cdata in elements), see ViewSchema#getTypeForName() where
> "text" and "html" are mapped to "string". That means the code (without your
> proposed changes) escapes \ to \\ for all three types, which is at least
> valid and useful for type="text" and type="html". (This implies your simple
> approach won't work.)
And it will do the wrong thing if people actually put non-ASCII unicode
characters in, won't it?
> One question is still left:
> How to handle type="string" (the real type, not "text" or "html")? Do we
> expect it to be an ECMAScript-string, that means to take the route in your
> change ("parse it as an ECMAScript string literal"). Or don't we want to
> treat it as an ECMAScript-string. In that case, unicode escape characters
> need to be written as XML-CharRefs &#xXXXX; instead of \uXXXX.
I think this is what we document, that LZX "string" == ECMAScript "String", so
I think we need at least this fix.
> I'd say we don't need to change anything, the user just used the wrong escape
> mode, he should have used XML-CharRefs instead of ECMAScript unicode escape
> sequences.
>
>
>
>
> On 5/25/2010 9:28 PM, P T Withington wrote:
>> My proposed change is only for<attribute type="string" />, which is
>> currently broken and the source of LPP-9027, so perhaps we should review
>> that separately.
>>
>> I agree that the handling of node text is a mess. A lot of the mess is due
>> to ancient cruft in the Text compiler that was only necessary for swf5 and
>> should be ripped out. Doing so would surely simplify the problem.
>>
>> I'm a little confused though: Am I missing something? I don't see where we
>> do any handling of attributes that might be declared to be of type "text" or
>> "html". Seems like it would be a great improvement if we did though. Maybe
>> we'd have less confusion if we called these types "cdata" and "content"?
>>
>> I can see also that the current mechanism for handling the content of a node
>> in NodeModel is broken, as you point out, because it is using
>> ScriptCompiler.quote on the values in addText. I agree this is incorrect.
>>
>> I propose 1) you review my change for it's limited fix, 2) we file an
>> improvement to rip out the swf5 text machinations, and 3) we file a bug
>> regarding NodeModel/addText doing incorrect escaping of node content.
>>
>> On 2010-05-25, at 11:02, André Bargull wrote:
>>
>>> I don't think this is right. If you set the value of an attribute with
>>> type="text", you need to use a 'xml-string' [1,2]. Also for Unicode
>>> characters you must not use a Javascript escape sequence \uXXXX, but the
>>> corresponding the character reference&#XXXX; [2].
>>> (People who don't like xml and xml character references should use
>>> type="expression" instead, that way they can write a 'javascript-string'
>>> [1] with Javascript escape sequences etc. :-) )
>>> I know this is a confusing topic [4], it's not always clear when to apply
>>> which escaping. But as said, also after reading the source code in
>>> NodeModel.java, it's clear that type="text" and type="html" should not
>>> treat the<attributes>'s value as a Javascript string. It might not be clear
>>> for type="text", but at least for type="html" it's obvious the value is a
>>> (x)html string, so no Javascript escape characters [5]!
>>>
>>> It's seems you want to have type="string" which treats the value as a
>>> Javascript string.
>>>
>>> And the change could possibly break applications because \ is no longer
>>> escaped to \\.
>>>
>>>
>>>
>>> [1] not an official term, only used here to make clear we've got
>>> 'xml-strings' and 'javascript-strings'
>>> [2] http://www.w3.org/TR/REC-xml/#NT-AttValue
>>> [3] http://www.w3.org/TR/REC-xml/#NT-CharRef
>>> [4] http://jira.openlaszlo.org/jira/browse/LPP-1948
>>> [5] the only difference between type="text" and type="html" is how to
>>> interpret text-content (= CharData
>>> http://www.w3.org/TR/REC-xml/#NT-CharData)
>>>
>>>
>>> On 5/25/2010 4:04 PM, P T Withington wrote:
>>>> Change 20100525-ptw-1 by [email protected] on 2010-05-25 09:56:35 EDT
>>>> in /Users/ptw/OpenLaszlo/trunk-2
>>>> for http://svn.openlaszlo.org/openlaszlo/trunk
>>>>
>>>> Summary: Correct quoting of string-typed attributes
>>>>
>>>> Bugs Fixed: LPP-9027 pattern attribute on text doesn't work as expected in
>>>> trunk
>>>>
>>>> Technical Reviewer: [email protected] (pending)
>>>> QA Reviewer: [email protected] (pending)
>>>>
>>>> Details:
>>>> lztest-node: Added tests to verify that setting a string
>>>> attribute using a literal or expression is the same as setting an
>>>> expression attribute to a literal string using the failing pattern
>>>> from the bug (which verifies the use of \u escapes to specify
>>>> unicode characters works).
>>>>
>>>> NodeModel: Before you quote a string literal, be sure to parse it
>>>> as an ECMAScript string literal (so that the ECMA \-escapes are
>>>> properly interpolated).
>>>>
>>>> Tests:
>>>> ant lztest with new node attribute tests
>>>>
>>>> Files:
>>>> M test/lztest/lztest-node.lzx
>>>> M WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java
>>>>
>>>> Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20100525-ptw-1.tar
>>>>
>>
>>
