P T Withington wrote: > This is some very tricky stuff! To the best of my ability, I reviewed > it technically, and it looks good to me. Since it is adding > functionality where it did not exist before, I approve checking it in so > we can get some use testing on it. > > I'd like your opinion on the risk/reward of integrating this to > branches/4.5? Again, since it is adding missing functionality, I lean > toward just putting it in.
I think it's pretty risk-free. Nobody's complained about the missing functionality so it's not likely anyone's missing it. Please merge if you see fit! > I have a few comments for your consideration, before you check in: > > 1. The name of the quirk 'text_selection_use_range' could be more > illuminating. Is this not _only_ for IE, which does not support DOM2 > createRange? Maybe it could be renamed something like > 'ie_selection_use_text_range'? We try to avoid tying quirks to browsers - in case another browser decides to pick up the bad habit. This isn't very likely in this case, but still... > 3. It seems you might be able to simplify getSelectionSize for multiline > in DOM2. Wouldn't it work to say: > > return range.toString().length; > > ? > > 4. Similarly, perhaps it would be simpler to compute > getSelectionPosition for multiline by: > > var container = document.createRange(); > container.selectNodeContents(range.startContainer.parent); > container.setEnd(range.startContainer, range.startOffset); > return container.toString().length; > > Just a thought, rather than looping through nodes. I tried that - but then the <br/> doesn't get counted as a character :( Newlines count as a character in Flash... > 5. Finally, is it important to call range.detach(), or will ranges be > detached when they get garbage-collected? Hmmm. Not sure. I believe so, at least I couldn't find anything on the Interweb about Ranges causing leaks in JavaScript. > On 2009-08-03, at 19:29EDT, Max Carlson wrote: > >> Change 20090803-maxcarlson-Z by [email protected] on 2009-08-03 >> 15:57:45 PDT >> in /Users/maxcarlson/openlaszlo/trunk-clean >> for http://svn.openlaszlo.org/openlaszlo/trunk >> >> Summary: Add support for text selection in DHTML >> >> Bugs Fixed: LPP-4106 - LzText getSelectionPosition, getSelectionSize >> not implemented in DHTML >> >> Technical Reviewer: ptw >> QA Reviewer: [email protected] >> >> Details: Add implementations of setSelection(), getSelectionSize() and >> getSelectionPosition(). >> >> Tests: See LPP-4106 >> >> Files: >> M WEB-INF/lps/lfc/kernel/dhtml/LzTextSprite.js >> >> Changeset: >> http://svn.openlaszlo.org/openlaszlo/patches/20090803-maxcarlson-Z.tar > -- Regards, Max Carlson OpenLaszlo.org _______________________________________________ Laszlo-reviews mailing list [email protected] http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews
