[Webkit-unassigned] [Bug 25444] SelectionStart, selectionEnd properties return wrong values when the selection is in a form input

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25444





--- Comment #20 from Julie Jeongeun Kim <jiyuluna at gmail.com>  2010-11-08 23:27:04 PST ---
Please check inline comment.

(In reply to comment #19)
> (From update of attachment 73179 [details])
> > WebCore/rendering/RenderTextControl.cpp:260
> > +bool RenderTextControl::isSelectableElement(Node* n) const
> Please do not abbreviate node as n.  See Names section in http://webkit.org/coding/coding-style.html.
OK. I'll change it to 'node'. 

> > WebCore/rendering/RenderTextControl.cpp:275
> I would write this as:
> Node* shadowAncestor = n->shadowAncestorNode();
> return shadowAncestor && (shadowAncestor->hasTagName(textareaTag)
>     || (shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField()));
OK. I'll change it as you recommended.

> > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:10
> > +PASS element.selectionStart is start
> > +PASS element.selectionEnd is end
> 
> These outputs aren't as useful as I'd like to be.  You might want to consider manually calling Passed / Failed to print out more useful information such as what offset was set and which offset was returned.  But I'm fine with this output if you're reluctant to change it.
In order to display useful information,
I have to send the second parameter of 'shouldbe' as a constant, not a variable.
It means I can't use function. I have to just spread out them like 'textarea-maxlength.js'.
If I have to do like that, I'll do it.

> Why don't you just pass element to startTest & testHandler instead of making if statement like this.
> i.e.
> testHandler(document.getElementById('text'));
> testHandler(document.getElementById('area'));
Because 'eval' in 'shouldBe' throws exception like 'ReferenceError: element is not defined.' if I use local variable or just argument, I put global variable and assign node to it.
As I told above,
If I change it to spread out, not using function,
This code will be removed.

> And please put a space before and after == and =.  Although this is JavaScript code, we'd like to keep it as clean as possible.
> 
> > LayoutTests/fast/forms/selection-start-end-readonly.html:35
> > +    start = s; end = e;
> 
> Why don't you name "s" and "e" "start" and "end" respectively in the first place?
Yes, I'll check it in test case as well.

> > LayoutTests/fast/forms/selection-start-end-readonly.html:44
> > +    var s = 0;
> > +    var e = 10;
> > +    startTest(type,s,e);
> 
> There's no need to declare s and e.  You can just do:
> startTest(type, 0, 10);
> If you follow my earlier comment, this should be:
> startTest(element, 0, 10);
> 
> You can also make an array of offset pairs and use a for-loop to go through all of them as in:
> 
> var offsets = [[0, 10], [0, 9], ...];
> for (var i = 0; i < offsets.length; i++)
>    startTest(element, offsets[i][0], offsets[i][1]);
It's same as the above.

In conclusion,
The 'eval' in 'shouldBe' throws exception if I use local variable or just argument.
I have to use constant at 'shouldBe' in order to show more useful information.
So, I'll change this test to spread out sentences like 'textarea-maxlength.js'.

Do you approve of my idea?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.