Bug 3401 – WebKit does not support setting selection range on form controls

• Status: VERIFIED
• Resolution: FIXED
• Component: Forms

[douglas phillips]

I have attached a screen shot for Mozilla Firefox which displays the partial text selection. Notice that in 
the text field I can programmatically select partial text. This is done with a call "setSelectionRange" 
method. For IE browser it starts with "createTextRange" call and ends with "select". If Safari can provide 
the same feature, it would be great. As of today, we can only do a complete text selection and is less 
useful than partial text selection.

[douglas phillips]

Created attachment 2197 [details]
showing partial text selection in Firefox

[Kevin Ballard]

Just to note, I'm working on this. I actually got this implemented (along with selectionStart and 
selectionEnd) for textareas and was working on doing it for inputs as well (text, password, and search 
types), when I learned that the KWQfoo classes are mimicing the Qt interface, so I'm going to need to re-
do part of my implementation.

Anyway, I'll hopefully have it done tomorrow.

[Kevin Ballard]

Created attachment 2520 [details]
A patch to fix this issue

[Maciej Stachowiak]

Comment on attachment 2520 [details]
A patch to fix this issue

Comments on the patch:

This does not match the coding style guidelines, the brace should be on a
separate line:

+static Value getInputSelectionStart(HTMLInputElementImpl &input) {

This code looks wrong, the toULong call attempts to turn a property name into a
number, so it won't match "selectionStart", rather it will match some arbitrary
integer. What you want to do to convert an identifier to a property int tag is
to use Lookup::findEntry. The Window object has an example of how to do it.
Right now, probably every input element appears to have the selectionStart and
selectionEnd properties, since the fallback goes to the special property
hashtable.

+      uint u = propertyName.toULong(&ok);
+      if (u == InputSelectionStart || u == InputSelectionEnd) {

For the following, maybe a #error directive or assert of some kind would work
better:

+    // FIXME: I can't test Qt, so I'm not going to bother
+    // trying to write the code for it

Wouldn't it be better to make no change at all for out of range values? What do
other browsers do when you give a negative selection start or end?

+    // The second MAX is a workaround for the unsigned aspect
+    // if range.location is larger, this will end up 0, instead of a huge
number

Besides these issues, the patch looks really good. Looking forward to the next
take.

[Kevin Ballard]

Created attachment 2532 [details]
A revised patch

This fixes all the mentioned issues. In the case of the MAX comment, that was
actually just a misleading comment. I've clarified the comment to better
explain the overflow situation.

[Darin Adler]

Comment on attachment 2532 [details]
A revised patch

It's not great to have kjs_html.cpp be checking the type enum of the HTML input
element. Instead HTMLInputElementImpl needs a DOM API that indicates whether
things like selectionStart and selectionEnd are appropriate. A separate
"canHaveSelection" function that returns a boolean would be one appropriate way
to do it.

As far as layout tests are concerned, you can make a test that uses these new
methods easily, and even though it doesn't change layout, it would be good.

In fact, we should change DumpRenderTree so it can dump the selection of form
elements just as it dumps a selection within HTML today, and then you could
make some even-better tests.

There's no need to have a break after a return in a case statement and it's
generally considered bad style.

It looks like some of the indenting is done with tabs. All the indenting should
be using spaces. In particular, the code in RenderLineEdit looks like it's
indented with a tab, as does the declaration of setSelectionRange in
HTMLTextAreaElementImpl in the header.

We usually say "unsigned" rather than "unsigned int"
(QLineEdit::setSelectionStart).

The biggest lack here is tests, though. There are many different problems you
are fixing here and edge cases you are checking for. All of them should have
individual tests.

Looks good! Keep at it.

[Kevin Ballard]

Created attachment 2539 [details]
the latest iteration of the patch

Ok, this patch should resolve all of the outstanding comments. It doesn't
include the tests, since I can't cvs add (anoncvs), so I've added the tests as
a separate attachment.

[Kevin Ballard]

Created attachment 2540 [details]
testcases

And here's the testcases. This has 2 different tests with expected output.

[Darin Adler]

Comment on attachment 2539 [details]
the latest iteration of the patch

Looking good! Here are some more comments:

The code in HTMLInputElementImpl::setValue that casts m_render to
RenderLineEdit* doesn't seem to check m_type. I don't think it's good to have
this code assume that storesValueSeparateFromAttribute() implies it's a
RenderLineEdit * even if it's true, especially without a comment. But it's easy
to fix, because updateFromElement() is a function that exists in RenderObject
and there's no need to cast m_render to call updateFromElement() on it.

The new code in this patch omits spaces beteen class names and *, using syntax
like RenderTextArea*. We use a space. This is mentioned in our coding style
guidelines under "Names" as rule #9.

I know that you investigated using the real Qt API for QLineEdit, but I'm still
not sure why you ended up adding KWQ-specific methods. It seems clear that
RenderLineEdit::selectionStart, for example, could just call selectionStart()
and then if that returned -1, call cursorPosition(). This patch errs too far on
the "don't worry about Qt, just do something KWQ-specific" side, I think, and
now that it's working, I think you should try a little harder to use the Qt API
except where there's a reason you can't. To give another example, you should
name the function setSelection as in Qt rather than setSelectionRange and have
the parameter types be int rather than long. The sole exception here might be
selectionEnd, since we might not want to require getting the selection text
just to compute the end point.

I still see some use of "unsigned int" where we customarily use "unsigned".

I don't see the value in your "just in case" check for NULL in the
getCursorPositionAsIndex:inParagraph: code. Either we should allow NULL for the
parameters (in case someone is interested in only one or the other), or we
should assert if they are NULL. We shouldn't add dead code "just in case". And
this change seems unrelated to the rest of the patch. Why not just leave it
out?

Do we know what the performance impact is of the new CRLF code? We might need
to test that.

Can we refactor setSelectionStart, setSelectionEnd, and setSelectionRange so
that we don't need so much duplicate code? Even better, lets just have
setSelectionRange (and name it setSelection to match Qt) and have the callers
do a tiny bit more of the work themselves.

[Darin Adler]

Just looked at the test cases. They look really good, but they don't test everything in this patch. For 
example: we need a test that checks that the new properties are undefined on at least one other type of 
<input> element, and tests that would fail without the new CRLF handling you added to various text area 
methods. If I remove part of your patch, it's important that at least one test case fails.

[Kevin Ballard]

updateFromElement() exists in RenderObject? When I searched for it I found individual implementations 
on the different types of RenderFoo classes. Besides, it only should be called when the value is stored 
separately from the attribute - I actually tried calling it at the end normally and it caused problems, I 
don't know why. But maybe I could just call it directly without the cast when the value is stored separate 
and see if it works.

As to omitting spaces, I just tried to copy similar calls I saw elsewhere, but I'll be happy to add spaces.

The reason I decided not to go with Qt was because it was a lot of work that was going nowhere. Qt's 
selection stuff is an index in a paragraph, not simply the range that both NSTextView and the javascript 
function wants. I would have ended up turning the NSTextView's range into paragraph/index-in-
paragraph positions, then turning them back into a range. And what's worse, the only way to turn them 
back into a range would be to ask for the length of every paragraph preceding the index, which would 
make KWQTextArea have to find each paragraph over and over (if you've read Joel on Software, think 
Schlemiel the Painter's algorithm). I thought it would be far better to just preserve the range as-is 
throughout the entire code path, and let Qt deal with the paragraph stuff if this code ever ends up back 
in khtml.

The NULL check in getCursorPositionAsIndex:inParagraph: was because Qt documents the cursor stuff 
as not doing anything if one of the inputs is 0, so I figured I'd simply not do anything rather than assert 
or return half of a useful value.

I don't know what the performance impact of the new crlf code is, but I can't imagine it's a hot path. 
The cursor stuff, AFAICT, is only used when updating the widget in some fashion (still not exactly sure 
how it's called), which doesn't seem like something that happens repeatedly. And the crlf handling stuff 
in the selection code obviously only affects calls to the new javascript properties/method, which again 
isn't a hot path.

I thought of refactoring the selection code to avoid duplicate code, but the implementations aren't quite 
the same. The KWQTextArea has to handle possible \r\n sequences before the selection, but the 
KWQTextFieldController doesn't. And the only other duplicate code is the stuff that checks the ranges 
for validity, and I don't see how that could really be factored out, especially since the maximum length 
is fetched differently between KWQLineEdit and KWQTextArea. I suppose I could write a function that 
takes a start, a length, and a maximum length and creates a valid range, but that doesn't seem terribly 
necessary, plus I don't quite know where it would go.

As for patches, yeah, I can test that selectionStart and selectionEnd are undefined on the button, but I 
can't test setSelectionRange because it's not undefined - I'm not sure how I make a function itself 
undefined on certain varieties of input. However, I could iterate over the button and make sure it's not 
in the property list, because I successfully hid it from that. Also, I don't know how to test the new crlf 
stuff. The crlf (and just cr) handling in the cursor position functions, I really don't know how to test, and 
as for the selection, there's no way for me to get a carriage return into a textarea in javascript anymore. 
I'm not even really sure if pasting it in by hand would work, and that would require manual interaction 
anyway.

[Darin Adler]

(In reply to comment #11)
> updateFromElement() exists in RenderObject?

Yes.

> As to omitting spaces, I just tried to copy similar calls I saw elsewhere, but I'll be happy to add 
spaces.

Excellent. Please do.

> The reason I decided not to go with Qt was because it was a lot of work that was going nowhere. Qt's 
> selection stuff is an index in a paragraph, not simply the range that both NSTextView and the 
javascript 
> function wants.

Good point for QTextEdit.

Doesn't apply for QLineEdit.

QLineEdit::selectionStart, QLineEdit::setSelection, QLineEdit::cursorPosition, and 
QLineEdit::setCursorPosition are all positions within all the text, not within a paragraph, so for that 
class I'd like you to write code that would work for both.

> The NULL check in getCursorPositionAsIndex:inParagraph: was because Qt documents the cursor
> stuff as not doing anything if one of the inputs is 0, so I figured I'd simply not do anything rather
> than assert or return half of a useful value.

Qt does not document the cursor stuff as "not doing anything". It says that you're not allowed to pass 0, 
which is not the same thing. Lets just leave this method alone.

> I don't know what the performance impact of the new crlf code is, but I can't imagine it's a hot path.

OK. Lets make a test.
 
> The cursor stuff, AFAICT, is only used when updating the widget in some fashion (still not exactly
> sure  how it's called), which doesn't seem like something that happens repeatedly. And the crlf
> handling stuff  in the selection code obviously only affects calls to the new javascript properties
> method, which again isn't a hot path.

We don't want to land a code change with no test, so if you can't figure out a way to test the "\r\n" 
changes to some methods, lets wait until we do have a way to test it other than code inspection.

> I thought of refactoring the selection code to avoid duplicate code, but the implementations aren't
> quite the same.

I think you misunderstood my suggestion. My suggestion was to refactor so that setSelectionStart, 
setSelectionEnd, and setSelectionRange, all within the same class, didn't contain so much duplicate 
code.

> As for patches, yeah, I can test that selectionStart and selectionEnd are undefined on the button, but I 
> can't test setSelectionRange because it's not undefined - I'm not sure how I make a function itself 
> undefined on certain varieties of input.

OK. I just want you to test what you've actually done. No need to make a test for something you didn't 
do!

> However, I could iterate over the button and make sure it's not in the property list, because I 
successfully hid it from that.

Great!

> Also, I don't know how to test the new crlf stuff. The crlf (and just cr) handling in the cursor
> position functions, I really don't know how to test, and as for the selection, there's no way
> for me to get a carriage return into a textarea in javascript anymore. 
> I'm not even really sure if pasting it in by hand would work, and that would require manual
> interaction anyway.

Then lets set these changes aside and discuss separately how to test them and whether to land them 
without tests.

[Darin Adler]

By "these changes" I mean the untestable CRLF changes that are independent of the rest of the patch. 
Ideally each patch fixes one bug, so landing these changes separately has other benefits as well.

[Kevin Ballard]

Ok, I can finally test the crlf stuff, but it requires manual interaction. I could probably automate it using 
my new selection stuff, except the selection stuff is in the patch as well so it wouldn't work prior to the 
patch. I guess I could submit it as a separate patch once the selection stuff is in.

[Kevin Ballard]

Created attachment 2549 [details]
Slightly modified testcases

I added an extra check to make sure selectionStart and selectionEnd were
undefined. I was already looping over the property list of the button to make
sure they weren't listed.

[Kevin Ballard]

Created attachment 2553 [details]
Yet another patch

Ok, this should fix the outstanding comments. I removed the fix for the crlf
handling in cursor position, because I can't test that without the selection
stuff already in place.

[Kevin Ballard]

The crlf fix for cursor position stuff now has its own bug, number 3654 (which is set dependent on this 
bug because its testcase requires selection manipulation).

[Darin Adler]

Comment on attachment 2553 [details]
Yet another patch

HTMLTextAreaElementImpl::setValue has an unnecessary cast in a call to
updateFromElement().

Otherwise looks great to me. We should remove that cast when landing this. r=me