Change 20100416-maxcarlson-O by [email protected] on 2010-04-16 16:15:14 PDT in /Users/maxcarlson/openlaszlo/trunk-clean for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: Clean up text measurement per Tucker's comments Bugs Fixed: LPP-8591 - Something mysterious about scrollHeight property in DHTML text kernel, see LzTextSprite.js Technical Reviewer: ptw QA Reviewer: hminsky Details: Feedback from Tucker: 1) Why in LzFontManager#23 do you use `LzFontManager.` instead of `this.` (as is used elsewhere)? Changed to use this. 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? Done 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. I changed to always use this.t/isprite.* 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. I suppose there's a small benefit in AS3 because the types of sprite isprite and tsprite are explicit. I unified to always use the properties instead of sometimes casting to the right type. 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. Conditionalizing the property based on the tag type appears to be required, at least in IE 7 which doesn't support pre-wrap. Internally, IE appears to be mapping newlines to <br/> with to innerText which is why the wrapping code works well. 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?o See http://jira.openlaszlo.org/jira/browse/LPP-8917. There is a capability (htmlinputtext) that should prevent calls for now. 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? I fixed it to restore the scroll value 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. It works in all supported browsers except IE7, where the use of innerText in __setTextContent() appears to work well. 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. Agreed. I pulled this from the original checkin because it was causing problems. 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? I clarified the comment. It's used for calls from user code, where the size may be needed before the sprite/view is initialized. LzFontManager - Use this instead of LzFontManager. dhtml/LzSprite - Remove unneeded null checks in setWidth/Height(). dhtml/LzInputTextSprite - Restore scrollTop to its old value. swf9/LzTextSprite - Set argument type of setWidth/Height/FontSize() to Number, remove unneeded null checks. swf9/LzSprite - Set argument type of setWidth/Height() to Number, set lzwidth lzheight and fontsize properties to type Number and correct null checks. LaszloView - Set type of font and fontstyle to String, correct type for fontsize and explain why the type can't be set. Remove unneeded set_alpha setter (checked demos and components). Prevent duplicate call to setWidth(). LzText - Use this.tsprite consistently. LzInputText - Use this.isprite consistently. Tests: smokecheck, lzpix, test/lztest/lztest-textheight.lzx and testcase from LPP-8813 all run as before across all runtimes. Files: 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/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 Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20100416-maxcarlson-O.tar
