Bug 6814

These bugs that block us switching to the new text field shoul not be marked P1. Once we turn it on, these would be P1/major bugs, of course, but at the moment they are just part of the "switch to a new text field" task and should not be in the P1 list.

<rdar://problem/4463736> Implement selection methods for RenderTextField (6814)

Created attachment 6910 [details] patch first cut at the selection methods. selectionStart and selectionEnd work well-- although there might be a faster/easier way to compute them. setSelectionRange doesn't quite work yet. I think it has something to do with who has focus after the selection is set. Not sure though. Also, this has a proposed fix for the following bug which was blocking testing of the selection methods. <rdar://problem/4463742> clicking on buttons or links outside of a contenteditable area causes selection to be lost

Created attachment 6926 [details] patch

Comment on attachment 6926 [details] patch Please use private instead of protected. No need to check for a document() of 0 in RenderTextField::selectionStart(). A renderer can't outlast its document. No need to explicitly convert to VisiblePosition in RenderTextField::selectionStart(). + int start = kMax(s, 0); Don't need that line of code, since getVisiblePositionForIndex already does the right thing. The setSelection helper does nothing if either position is null. I don't think that's helpful behavior. I suggest asserting that neither is null, and further, I suggest asserting that each is within the text field's generated content. I don't think you need a separate function for this. It could be folded into setSelectionRange. Frame needs a function that does setSelection after doing shouldChangeSelection and you should be calling that. getVisiblePositionForIndex is willing to advance past the end of the div. That's not good -- if the caller of setSelectionRange passes a large value for end you'll end up selecting "off the end". If you change it to instead return the end for those values, then it's guaranteed to never return null, which makes it easier to use. Normally we leave out the word "get" from function names like these. getIndexForVisiblePosition should take a const VisiblePosition& rather than a VisiblePosition. You can make a faster version of getVisiblePositionForIndex by using CharacterIterator. You can make a much faster version of getIndexForVisiblePosition by composing a range and calling TextIterator::rangeLength. I noticed that clearSelectionIfNeeded needs to be a private function in DocumentImpl. As we discussed, we need to document what setFocusNode does clearly to figure out when it should and should not clear the selection. Once we understand that we can figure out whether a boolean parameter is an appropriate way to prevent it from doing so. The change to the logic about clearing selections in dispatchMouseEvent is not clearly correct. We need more research to decide what rule we're trying to implement.

Created attachment 6948 [details] patch to address Darin's comments I think this patch addresses Darin's comments. I've removed the setFocusNode changes, since those issues are tricky and still need to be worked out.

Comment on attachment 6948 [details] patch to address Darin's comments ugh. I'm hitting the assert when the text field is invisible.

Comment on attachment 6948 [details] patch to address Darin's comments On second thought, I'm going to file another bug about this loose end with selection failing with hidden text fields. I'd like to fix that in a separate checkin.

Two related bugs filed: http://bugzilla.opendarwin.org/show_bug.cgi?id=7675 When new text fields change from visibility:hidden to visibility:visible, value doesn't display and http://bugzilla.opendarwin.org/show_bug.cgi?id=7676 Selection methods on new text fields don't work if text field is hidden

Comment on attachment 6948 [details] patch to address Darin's comments a) + int indexForVisiblePosition(const VisiblePosition& pos); No need for the name pos here; I would leave it out. b) + setSelectionRange(selectionStart(), end); If you set an end that is < the start with setSelectionEnd, should it move the start back too? c) + end = kMax(end, start); More use of kMax is deprecated. Instead you should use the standard max function template from <algorithm>. Just make sure something includes <algorithm> and something does "using namespace std" and then call max instead of kMax. d) If m_div is 0, then RenderTextField::setSelectionRange is going to fail its assertions. The actual function looks like it will just change the selection to nothing. e) RenderTextField::setSelectionRange always calls shouldChangeSelection even if there's no selection change; that doesn't seem good. I think there is a good argument for adding a function to frame that first checks if the new selection is any different from the old, then calls shouldChangeSelection and then setSelection. f) It's kind of annoying that CharacterIterator won't let you call advance if you're already at the end, because it does let you try to advance past the end and handles that correctly by pinging you at the end. At some point, we should fix that so you can remove the atEnd() check here. g) + return VisiblePosition(it.range()->endContainer(ec), it.range()->endOffset(ec), DOWNSTREAM); Maybe that should be UPSTREAM? I'm not sure. These are all minor, so I'm marking this review+. The most important issue is (d), and even that is an issue in debug versions only. I'd like you to add some tests about how setSelectionRange behaves when m_div is 0, but I don't understand under what circumstances it can be 0.

After looking at the code more to fully understand when m_div gets initialized, I found that updateFromElement gets called right as the RenderTextField is being attached, so by the time any of the other methods are called on the renderer, m_div should already be initialized.