Approved! On 2010-04-16, at 20:16, Max Carlson wrote:
> 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 >
