[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 #19 from Ryosuke Niwa <rniwa at webkit.org>  2010-11-08 11:29:38 PST ---
(From update of attachment 73179)
View in context: https://bugs.webkit.org/attachment.cgi?id=73179&action=review

This patch looks much better but needs a little more polishing.

> 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.

> WebCore/rendering/RenderTextControl.cpp:275
> +    if (Node* shadowAncestor = n->shadowAncestorNode()) {
> +        if ((shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField()) || shadowAncestor->hasTagName(textareaTag))
> +            return true;
> +    }
> +    return false;

I would write this as:
Node* shadowAncestor = n->shadowAncestorNode();
return shadowAncestor && (shadowAncestor->hasTagName(textareaTag)
    || (shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField()));

> 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.

> LayoutTests/fast/forms/selection-start-end-readonly.html:24
> +    if (type == 1)
> +        element= document.getElementById('text');  
> +    else if (type ==2)
> +        element= document.getElementById('area');  

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'));

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?

> 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]);

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