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 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'?
2. I don't think you need the quirksmode workaround for Safari not
having getRangeAt, that was back at Safari 1. It seems to have it
now. I suppose it doesn't hurt, other than cluttering the code.
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.
5. Finally, is it important to call range.detach(), or will ranges be
detached when they get garbage-collected?
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
_______________________________________________
Laszlo-reviews mailing list
[email protected]
http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews
