[Webkit-unassigned] [Bug 60529] Programmatically set selection should not have direction on Mac
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 12 23:20:33 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=60529
--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> 2011-07-12 23:20:32 PST ---
(From update of attachment 100621)
View in context: https://bugs.webkit.org/attachment.cgi?id=100621&action=review
Great to see more test coverage but I feel like we need few more iterations.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-expected.txt:73
> +Superfluous line.
> +Superfluous line.
> +The first sentence. The second sentence. Three sentences.
> +Another sentence. A fifth sentence. A sixth sentence.
> +Sentence seven. Sentence eight. The last sentence.
> +Superfluous line.
> +Superfluous line.
> +Superfluous line.
> +Superfluous line.
> +The first sentence. The second sentence. Three sentences.
> +Another sentence. A fifth sentence. A sixth sentence.
> +Sentence seven. Sentence eight. The last sentence.
> +Superfluous line.
> +Superfluous line.
You should probably hide this one the test is done in DRT (only if window.layoutTestController is defined)
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:14
> +function setDivSelection() {
"set" is very general term. You should say which word or character you're selecting instead.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:27
> +function setTextSelection() {
Ditto about the function name.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:30
> + container.selectionStart = 114;
> + container.selectionEnd = 119;
Can we avoid having these magic numbers somehow? Why are they so large?
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:33
> +function check(direction, test, expectation) {
"check" is a very general name. Please give a more descriptive name.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:35
> + sel = window.getSelection();
Please do not use abbreviation.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:45
> + if (direction == "forward" && sel.baseOffset < sel.extentOffset)
> + pass = true;
> + else if (direction == "backward" && sel.baseOffset > sel.extentOffset)
> + pass = true;
> + else
> + pass = false;
All of this can be a single statement:
pass = (direction == "forward" && sel.baseOffset < sel.extentOffset)
|| (direction == "backward" && sel.baseOffset > sel.extentOffset);
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:60
> + if (String(sel) != expectation) {
> + debug("found....\'" + String(sel) + "\'");
> + pass = false;
> + }
> + else if (pass != false) {
> + pass = true;
> + }
> +
> +
> + if (pass == true)
> + testPassed(description + " in " + testId);
> + else
> + testFailed(description + " in " + testId);
These two should be combined. e.g.
if (selection.toString() == expectation)
testPassed(description + " in " + testId);
else
testFailed(description + " in " + testId + ' expected "' + expectation + '" but got "' + selection.toString() + '"');
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:71
> + if (testId == "regular-div" || testId == "editable-div")
It seems like we should be checking element's localName of the test node.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:88
> + str += extendSelection();
> + str += ", then ";
> + str += extendSelection();
> + str += ", then ";
> + str += retractSelection();
Why are you extending selection thrice? Could you clarify as to why this is needed? It seems like we just need to extend once to test this. The thing is, in this patch, the very first extend is very important and we should be checking the result of that rather than the behavior after 3 extend's.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:94
> +testId = ""; // This is global, rather than passing it to ugly places.
> + // It specifies what element we're testing: a regular div, a content-editable div, a textarea, or a text-input field.
I'd rather give the test node. Also if we had called it currentTestNode, this comment can be removed.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:110
> + if (testId == "text-area")
> + expectedResult = "Another sentence. A fifth";
> + else if (testId == "text-input")
> + expectedResult = " sentence. A sixth sentence. Sentence seven. Sentence eight. The last sentence. Superfluous line. Superfluous line.";
> + else
> + expectedResult = "e second sentence. Three sentences.\nAnother sentence. A fifth";
> +
> + check("backward", makeTest("left", "line"), expectedResult);
Just looking at this code, I can't tell why we expect these results because I can't see what the initial setup was.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:132
> +function winTests(whichElement) {
> + testId = whichElement;
> + window.layoutTestController.setEditingBehavior("win");
> + tests();
> +}
> +
> +function unixTests(whichElement) {
> + testId = whichElement;
> + window.layoutTestController.setEditingBehavior("unix");
> + tests();
> +}
It seems like these two functions are unnecessary. See below.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:134
> +function tests() {
"tests" is a very general name. Please give a more descriptive name. Also, I don't understand why we have tests and macTests.
I'd like to see the expected results for mac and windows/unix next to each other so that I can tell what the difference is.
I've used the following pattern:
runTestAndVerify(... {'Win': 'hello': 'Unix': 'hello', 'Mac': 'world'}[platform])
where "hello" is the expected for Win/Mac editing behaviors and "world" is the expected for Mac.
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:162
> +function run(platformTest) {
> + platformTest("regular-div");
> + platformTest("editable-div");
> + platformTest("text-area");
> + platformTest("text-input");
> +}
This function can just take editingBehavior string e.g. Mac, Win, and Unix as in:
function runTest(platform) {
debug(platform + ':');
layoutTestController.setEditingBehavior(platform);
tests("regular-div");
tests("editable-div");
tests("text-area");
tests("text-input");
}
Also, why don't you just traverse child nodes of the container and call tests() on each child instead of giving explicit id and repeating ids here?
i.e. do something like (of course tests currently take an id so you'd have to change that):
for (var i = 0; i < container.childNodes.length; i++)
tests(container.childNodes[i]);
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:166
> +container = document.createElement('div');
> +document.body.appendChild(container);
> +container.innerHTML =
Is there a point in appending this container dynamically? Can't we just include it in the markup?
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:173
> + debug("Beginning mac tests...");
"Beginning " and "..." are unnecessary. Just say "Mac tests:".
> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:177
> + debug("Beginning windows tests...");
> + run(winTests);
> + debug("Beginning unix tests...");
Ditto for Windows and Unix.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list