[Sorry for the delay in review, I see you have already checked in, but I think
there are some important issues yet to be addressed:]
Nits:
1) Why in LzFontManager#23 do you use `LzFontManager.` instead of `this.` (as
is used elsewhere)?
2) In swf9/LzTextSprite/setWidth, setHeight: if the parameter must be a number,
shouldn't we declare it so (:Number instead of :*) so we get enforcement from
the Flex compiler?
3) In LzInputText you changed `this.isprite.getTextfieldHeight` to
`this.getTextHeight`, yet in LzText you changed `this.getTextWidth` to
`this.tsprite.getTextWidth`. It seems like we ought to have a style rule about
which to prefer and stick with it.
4) We ought to have a clean-up task to consolidate sprite, tsprite, and
isprite. They are all initialized to the same sprite AFAICT, so I don't know
why we have 3 different names. Just seems confusing to me.
Issues:
1) We still have the issue that measure divs are all divs, yet we are calling
__setTextContent to fill them in. There's a disconnect here. We are
dispatching on the tagname (because that is more efficient than asking the DOM
the node type), but the tagname and node type do not correspond in this case.
(The correspondence is correct for the real sprite, but not for this
measurement pseudo-sprite.) We need to go one way or the other: either finish
the job of using the correct DOM node for the measurement sprite, or revert to
the previous situation where we just use 'div' for measuring and use 'div' as
the tagname for the cache and _setTextContent parameter.
2) I'm concerned about this comment in __setTextContent:
> // NOTE: [2009-03-29 ptw] For now, DHTML does not support HTML in
> // input text, so we must measure accordingly
in relation to the separate discussions about HTML in <inputtext>. Is this a
trap waiting for us? Worthy of a separate Jira entry?
3) At LzInputTextSprite#747, what is this for:
> d.scrollTop = 0;
If this is a temporary setting needed for measurement, shouldn't it be restored?
4) At LzSprite#472, we now believe that `whitespace: pre-wrap` works correctly
in all target browsers? It was not used before because its implementation was
spotty.
5) I do not believe LaszloView#updateWidth, updateHeight should be doing
anything with the _set_{widht,height}_memo. Consider: the user sets to `null`
to get auto-sizing, then reads the width, then sets the width to the read value
to fix the size. Because update will have smashed the memo field, the setter
will skip out early, when it should be running to completion because the node
should switch from auto-sizing to fixed size. The memo field is intended only
to short-circuit redundant setter calls, we should not be trying to overload it
with other functionality.
6) I still don't understand the need for the `force` argument to
getTextfieldHeight and getTextWidth. It seems that all it will do is
contravene the test to _not_ try to measure the sprite if it is not inited,
which seems like a bad thing. Can you enlighten me?
On 2010-04-14, at 18:38, 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 ^2 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, LPP-7832 - Kernel API getTextHeight should be renamed getLineHeight to
> avoid confusion
>
> Technical Reviewer: hminsky
> QA Reviewer: ptw
> Doc Reviewer: (pending)
>
> Documentation:
>
> Release Notes:
>
> Overview: Updated to address Tucker's comments:
>
> 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)
>
> I updated all 0px values to 0 and changed LzKernelUtils.CSSDimension() to
> return 0 as-is.
>
> 2) I think a better idiom for (m == true) is (!!m). My 2p.
>
> Changed.
>
> 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?
>
> I added a comment.
>
> 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).
>
> Yeah, you're right. I managed to eliminate or rationalize all internal sprit
> e.setWidth/Height() calls, since _updateSize() was updating that with
> definitive values anyway.
>
> I made sure LzText.getTextfieldHeight/TextWidth() notified the kernel to
> bypass text initialization options.
>
> 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?
>
> Good catch! That is what it was doing before because getTextHeight() ->
> getLineHeight().
>
> 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?
>
> Yes, this has been fixed. Perhaps we'd get better measurements if we used
> actual <input or <textarea objects?
>
> 4) The comment at LaszloView#2440 is garbled. Same at LaszloView#2473
>
> Fixed.
>
> Otherwise, the same:
>
> 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: 0px -> 0, store
> scrolldivtagname for faster lookups, remove unneeded calls to
> setWidth/Height() since __updatefieldsize covers them, optimize style
> measurement key lookups, move text measurement to LzFontManager, fix
> setWidth() to account for text-indent per Andre's comments, skip unneeded
> updates to scroll div in setHeight(),
>
> LzFontManager - Move text measurement from LzTextSprite
>
> WEB-INF/lps/lfc/kernel/swf/LzTextSprite.as: Rename getTextHeight ->
> getLineHeight. Add new 'force' arg to getTextfieldHeight (unused in this
> implementation, but for compatibiltiy
> with DHTML)
>
> WEB-INF/lps/lfc/kernel/swf9/LzTextSprite.as: Rename getTextHeight ->
> getLineHeight. Add new 'force' arg to getTextfieldHeight (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), 0px -> 0, default lzswfinputtextmultiline CSS
> to whiteSpace: 'pre-wrap', use LzFontManager.__createContainerDiv() to create
> font measurement container.
>
> 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. Track scrolldivtagname, rely on
> view.inputtextevent to call __updatefieldsize(), 0px -> 0.
>
> LzKernelUtils: Return 0 immediately in CSSDimension()
>
> 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. Call sprite APIs directly for text size
> information, and pass flag for getTextHeight() so sprites know when the call
> is from user code.
>
> 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: Updated warnings to reflect current state
> in different runtimes
>
> 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/LzFontManager.js
> 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/LzKernelUtils.lzs
> 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
>
