I'm not a listed reviewer, but I have a couple of questions:
Nits:
1) In CSS, you don't need to specify units for `0`: `0px` is superfluous (In
LzSprite.js). So is `"0px"`, and `0+"px"`! (In LzIputTextSprite.js)
2) I think a better idiom for (m == true) is (!!m). My 2p.
3) I guess you are saying that it is more efficient to store the tagname as a
sprite property than to ask for it back from the DOM? Perhaps a comment to
that effect so future generations do not wonder?
Issues:
1) Why do we need this {setWidth,setHeight}/skipupdate flag? Is there ever a
time that the generic LFC calls setHeight that it would need a callback from
the kernel to actually install the height? It would seem better to me to fix
up the sprite implementation to not use the LFC->kernel API internally when it
is trying to send an update the other way (which is what the old `force` flag
was for).
2) I don't understand the change in LzTextSprite/__fontloaded from setting the
height to the height of the contents to setting it to just 1-line high. Why?
3) I realize this is not part of your change, but I am concerned about the
TODO on line 529 ff of LzTextSprite. It seems we _have_ implemented using the
correct tag for the measurement node, but we _have_not_ made that tag part of
the cache key (the cache key is hardwired to 'div'). Isn't that going to screw
up measurement if we happen to have a <text> and <inputtext> with the same
content? Shouldn't the `'div'` on line 533 be `this.scrolldivtagname` instead?
4) The comment at LaszloView#2440 is garbled. Same at LaszloView#2473
On 2010-04-13, at 23:08, Max Carlson wrote:
> Change 20100410-hqm-e by [email protected] on 2010-04-10 19:31:18 EDT
> in /Users/hqm/openlaszlo/trunk
> for http://svn.openlaszlo.org/openlaszlo/trunk
>
> Summary: UPDATED BY MAX: make autosizing inputtext recompute it's height
> properly when user types
>
> New Features:
>
> Bugs Fixed: LPP-8591 Something mysterious about scrollHeight property in DHTML
>
> Technical Reviewer: max
> QA Reviewer: (pending)
> Doc Reviewer: (pending)
>
> Documentation:
>
> Release Notes:
>
> Overview: I tweaked this change a bit in order to make it more efficient. I
> also renamed some args. Now test/lztest/lztest-textheight.lzx lists all
> expected warnings, and things look more visually consistent across dhtml,
> swf8 and swf10. And of course, variable-height inputtexts resize when you
> type in them. And, I found and fixed a text measurement bug in IE where the
> first time a string was measured, it could be inaccurate.
>
> + make autosizing inputtext recompute it's height properly when user types
>
>
>
> Details:
>
> WEB-INF/lps/lfc/kernel/dhtml/LzTextSprite.js:
> Needed to add a 'skipupdate' arg to setHeight, to prevent it from
> calling back up to the LFC with the updateSize() method.
>
> WEB-INF/lps/lfc/kernel/swf/LzTextSprite.as:
> Add new 'skipupdate' arg to setHeight (unused in this implementation, but for
> compatibiltiy
> with DHTML)
>
> WEB-INF/lps/lfc/kernel/swf9/LzTextSprite.as:
> Add new 'skipupdate' arg to setHeight (unused in this implementation, but
> for compatibiltiy
> with DHTML)
>
> WEB-INF/lps/lfc/kernel/dhtml/LzSprite.js:
> Added quirk for setting height to zero in order to get proper scroll height
> in IE and Safari
> (see __textEvent() in dhtml/LzInputTextSprite.js)
>
> WEB-INF/lps/lfc/kernel/dhtml/LzInputTextSprite.js
> When a multiline field is supposed to be autosize it's height, update the
> position for IE and Safari quirk, when user types into the field.
>
> WEB-INF/lps/lfc/views/LzInputText.lzs:
>
> When 'onchange' event is received from kernel, multiline field
> should update it's height to the measured text height, using
> 'updateHeight' instead of setHeight (so it doesn't turn it into a
> fixed height field)
>
>
> WEB-INF/lps/lfc/views/LzText.lzs:
> Autosizing text fields should call updateHeight, not setHeight, when updating
> their height, so they don't turn into fixed height fields.
>
>
> WEB-INF/lps/lfc/views/LaszloView.lzs:
> updateHeight and updateWidth methods update the memo cache vars
>
> WEB-INF/lps/server/src/org/openlaszlo/sc/CommonGenerator.java:
> + fix for an unrelated bug that was picked up by findBugs analysis
>
> WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java:
> + need to tell the JDOM XML parser to preserve newlines in text.
>
>
> test/lztest/lztest-textheight.lzx:
>
> Retained all warnings,
>
> Tests: See LPP-8591
>
> Look at text and image sizing in:
> demos/lzpix/app.lzx swf,swf10,dhtml
> calendar demo
> amazon demo
> test/lztest/lztest-textheight.lzx
>
> test/smoke/smokecheck
>
> Files:
> M test/lztest/lztest-textheight.lzx
> M WEB-INF/lps/lfc/kernel/swf/LzTextSprite.as
> M WEB-INF/lps/lfc/kernel/dhtml/LzSprite.js
> M WEB-INF/lps/lfc/kernel/dhtml/LzTextSprite.js
> M WEB-INF/lps/lfc/kernel/dhtml/LzInputTextSprite.js
> M WEB-INF/lps/lfc/kernel/swf9/LzTextSprite.as
> M WEB-INF/lps/lfc/kernel/swf9/LzSprite.as
> M WEB-INF/lps/lfc/views/LzInputText.lzs
> M WEB-INF/lps/lfc/views/LzText.lzs
> M WEB-INF/lps/lfc/views/LaszloView.lzs
> M WEB-INF/lps/server/src/org/openlaszlo/sc/CommonGenerator.java
> M WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java
>
> Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20100410-hqm-e.tar
>
