[Laszlo-reviews] [UPDATED^2] For Review: Change 20100525-ptw-1 Summary: Correct quoting of string-typed attributes

[André Bargull]

ViewSchema#getTypeForName():
>          if (name.equals("text") ||
>              name.equals("html"))
> -            name = "string";
> +            name = "text";

A bit simpler:
if ("html".equals(name)) {
     // [insert explanatory comment here]
     name = "text";
}


> +    html: StringPresentationType,
> +    text: TextPresentationType,

1) If text and html are treated as synonyms for now, they should have 
the same presentation type.
2) test/smoke/presentation-types.lzl needs to be updated to test the new 
types, too.


In TextPresentationType.present():
> +          i = e + 1;

i = e? e is the position of the ';', e+1 is the next character which 
needs to be processed, but as the for-loop increments i another time, 
the next character which is processed is at position e+2, so the char 
value[e+1] is left out.


NodeModel#compileAttribute():
> +                // Immediate string, token, and id types are
> +                // auto-quoted, but must _first_ be parsed as ES
> +                // strings!

I'm not sure whether this is valid for IDs and tokens. Have you tested this?
For example this test doesn't compile at all, that means the compiler 
doesn't not parse the values as ES strings:
<canvas>
<!-- name's type is token, id's type is ID -->
<view name="\x60" id="\x61"/>
</canvas>


> +            } else if (type == ViewSchema.XML_CDATA_TYPE
> +                       || type == ViewSchema.XML_CONTENT_TYPE) {
> +                // Immediate text and html types are auto-quoted; They
> +                // will be treated differently in the runtime
> +                // PresentationType system

This code is a bit misleading, because right know html-type is 
auto-converted to text-type (ViewSchema#getTypeForName() !). An 
additional comment should be added for clarification.


Some components need to be updated to change the type from string to 
text for backward compatibility. I'd suggest to do this in a subsequent 
change set. For example in lz/windowpanel.lzx, the "title" attribute is 
set to string, but it's only used in a constraint for a <text> element. 
I think in this case the type for "title" should be changed to text.


And it'd be nice if docs/src/developers/methods-events-attributes.dbk 
was updated to reflect the new understanding of string/text/html.



On 6/12/2010 9:48 PM, P T Withington wrote:
> [UPDATED:
>
> Per André's last suggestion, I am not trying to sort out the text/html issues here.  I am only splitting string from text/html, so that attributes with type string can have literal values expressed as ECMAScript String literals.
>
> ]
>
> Change 20100525-ptw-1 by ptw at padme.home on 2010-05-25 09:56:35 EDT
>      in /Users/ptw/OpenLaszlo/trunk
>      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: andre.bargull at udo.edu (pending)
> QA Reviewer: mdemmon at laszlosystems.com (pending)
>
> Release Notes:
>
>      The LZX `string` attribute type is no longer a synonym for `text`
>      and `html`.  The `string` type implies an ECMAScript string.
>      Literal values assigned to attributes of that type will be
>      interpreted as ECMAScript `String` literals, meaning that the
>      ECMAScript single-character (\t, \n, etc.) and short (\xNN) and
>      long (\uNNNN) escapes will be interpreted as defined in the
>      ECMAScript standard.  The `text` and `html` types imply XML
>      content.  Literal values assigned to attributes of that type will
>      (continue to) be interpreted as verbatim text.  ECMAScript escapes
>      will not be interpreted.
>
>      The `<text>/text` attribute is now declared to be of type `text`,
>      to indicate that it represents HTML content, not an ECMAScript
>      string.  If you redeclare the `text` attribute in a subclass of
>      `<text>` you should omit any type declaration as it is redundant.
>      (If you had declared the type as `string`, the compiler will issue
>      a warning.)
>
> Overview:
>
>      Split the `string` LZX type from the `text` and `html` types
>      (leaving the latter two as synonyms for now).  Declare the type of
>      `<text>/text` to be `text` (which is the popular concensus in
>      components, although perhaps incorrect because<text>  behaves as
>      if it were `html`; that is, it interprets markup).
>
>      With this split, we are able to have the LZX compiler interpret
>      literal `string` values as ECMAScript `String`s and solve the
>      original bug.  In a future change we will address the
>      `<text>/text` `text`/`html` issue.
>
> 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).
>
>      LzNode:  Added mapping for text and html presentation types..
>
>      PresentationTypes:  Added text presentation type which
>      escapes/unescapes the 5 XML entities.  This presentation type
>      should eventually replace the many copies of xml escaping code.
>
>      LzText:  Change the @lzxtype of the `text` attribute from `string`
>      to `text`.
>
>      LzDataset:  Correct spelling of @lzxtype noticed in passing
>
>      SchemaBuilder:  Handle `text` and `html` types
>
>      Schema, ViewSchema, NodeModel: Added real types for 'text' and
>      'html', distinct from 'string', which affects how immediate values
>      of those types are interpreted by the tag compiler: 'string'
>      implies an ECMAScript string (i.e., interpreted as an ECMAScript
>      String literal), 'text' implies XML CDATA and 'html' implies XML
>      content.  In this change, the latter two remain synonyms (as they
>      currently are), and are _not_ interpreted as ECMAScript strings,
>      but as XML content.  In a later change the implied distinction
>      between `text` and `html` will be addressed
>
>      debugger:  Remove unused `escapeText` noticed in passing.
>
>      basecombobox: Remove redundant (conflicting) attribute
>      declaration.
>
>      photo, classes: Remove redundant (conflicting) attrbute type
>      specification.
>
> Tests:
>      ant lztest with new node attribute tests, smokecheck, various
>      combinations of demos and runtimes.
>
> Files:
> M       test/lztest/lztest-node.lzx
> M       WEB-INF/lps/lfc/core/LzNode.lzs
> M       WEB-INF/lps/lfc/core/PresentationTypes.lzs
> M       WEB-INF/lps/lfc/views/LzText.lzs
> M       WEB-INF/lps/lfc/data/LzDataset.lzs
> M       WEB-INF/lps/server/src/org/openlaszlo/js2doc/SchemaBuilder.java
> M       WEB-INF/lps/server/src/org/openlaszlo/xml/internal/Schema.java
> M       WEB-INF/lps/server/src/org/openlaszlo/compiler/ViewSchema.java
> M       WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java
> M       lps/components/debugger/debugger.lzx
> M       lps/components/base/basecombobox.lzx
> M       demos/lzpix/classes/photo.lzx
> M       demos/lzpix/classes/classes.lzx
> M       demos/lzpix/classes/clipboard.lzx
>
> Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20100525-ptw-1.tar
>