60529 – Programmatically set selection should not have direction on Mac

Ryosuke Niwa

Reported 2011-05-09 20:50:22 PDT

WebKit on Mac has a bug that selection set programmatically has forward direction (i.e. base is before extent). On TextEdit, finding text in a document sets a non-directional selection on found texts.

Justin Garcia

Comment 1 2011-05-09 20:56:51 PDT

<rdar://problem/9411324>

Ryosuke Niwa

Comment 2 2011-06-27 11:36:45 PDT

It seems like we should move m_isDirectional from FrameSelection to VisibleSelection because undo/redo also needs to preserve m_isDirectional.

Wyatt Carss

Comment 3 2011-07-11 12:21:06 PDT

Created attachment 100347 [details] Patch

Ryosuke Niwa

Comment 4 2011-07-11 12:36:30 PDT

Comment on attachment 100347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100347&action=review It's nice that the code change is exactly what I had anticipated but I'd like to see tests for undo/redo. When a user editing action or execCommand is undone or redone, selection restored by WebKit should be directionless on Mac. Similarly, you should run the test cases you have on input/textarea. > LayoutTests/editing/selection/correct-selection-direction-for-platform-expected.txt:1 > +This test ensures that the direction of a programmatically set selection is correct depending on the platform. On mac, a programmatic selection should have no default direction, while on all other platforms, the direction should be forward. The expected results for this test are platform independent, due to the use of SetEditingBehaviour. "correct" seems too vague. You should say the programming set selection is directionless on and only on Mac. Also, "direction" is a over-loaded word in WebKit because it usually refers to the text direction (ltr/rtl) but can also refer to writing mode such as vertical-lr. Saying that the expected results for this platform are independent is unnecessary and should be removed. > LayoutTests/editing/selection/correct-selection-direction-for-platform-expected.txt:6 > +PASS selection is ' second', with base: 30, and extent: 23. This output isn't so useful because it doesn't tell me what action/function call made this selection. > LayoutTests/editing/selection/correct-selection-direction-for-platform.html:1 > +<!doctype html> While HTML5 spec says it's case-insensitive, I'd prefer to have <!DOCTYPE html> > LayoutTests/editing/selection/correct-selection-direction-for-platform.html:15 > + s = window.getSelection(); Please avoid using one-letter variable. > LayoutTests/editing/selection/correct-selection-direction-for-platform.html:22 > +function setup(testContainer, start, end, result) { Why does this function take "result" and never use it? > LayoutTests/editing/selection/correct-selection-direction-for-platform.html:46 > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'left', 'character', 30, 23, ' second'); I would have combined setup and test. A general pattern I used in these tests is that I'll have an "actor" closure that sets up selection & calls Selection.modify. It then returns the string explaining what it did. The test function then compares the current selection to the expected results and prints out the pass/fail along with the string returned by the "actor". > LayoutTests/editing/selection/correct-selection-direction-for-platform.html:87 > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'left', 'character', 24, 29, 'secon'); > + test('extend', 'right', 'character', 24, 30, 'second'); > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'left', 'word', 24, 24, ''); > + test('extend', 'right', 'word', 24, 30, 'second'); > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'left', 'sentence', 24, 20, 'The '); > + test('extend', 'right', 'sentence', 24, 41, 'second sentence. '); > + > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'right', 'character', 24, 31, 'second '); > + test('extend', 'left', 'character', 24, 30, 'second'); > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'right', 'word', 24, 39, 'second sentence'); > + test('extend', 'left', 'word', 24, 31, 'second '); > + setup(testContainer, 24, 30, 'second'); > + test('extend', 'right', 'sentence', 24, 41, 'second sentence. '); > + test('extend', 'left', 'sentence', 24, 20, 'The '); I would have made this a function, and just called it twice after calling setEditingBehavior as in: setEditingBehavior('win'); runTest(); setEditingBehavior('linux'); runTest();

Ryosuke Niwa

Comment 5 2011-07-11 12:37:06 PDT

Comment on attachment 100347 [details] Patch r- due to inadequate tests and various nits.

Wyatt Carss

Comment 6 2011-07-12 21:23:50 PDT

Created attachment 100621 [details] Patch

Wyatt Carss

Comment 7 2011-07-12 21:26:32 PDT

The uploaded patch still needs undo/redo tests, but I want to settle on a format for these first, since the undo/redo ones will be similarly set up and executed. Please review! Thanks :)

WebKit Review Bot

Comment 8 2011-07-12 21:52:33 PDT

Comment on attachment 100621 [details] Patch Attachment 100621 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9017410 New failing tests: editing/deleting/delete-hr.html editing/deleting/delete-3608445-fix.html editing/deleting/delete-br-009.html editing/deleting/delete-br-005.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-br-007.html editing/deleting/delete-and-undo.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/deleting/delete-3608462-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/execCommand/paste-1.html editing/execCommand/find-after-replace.html editing/deleting/delete-at-start-or-end.html editing/deleting/delete-br-006.html editing/deleting/delete-br-010.html editing/deleting/delete-4083333-fix.html editing/execCommand/format-block.html editing/deleting/delete-br-002.html editing/deleting/delete-character-001.html editing/deleting/delete-br-004.html

WebKit Review Bot

Comment 9 2011-07-12 21:52:39 PDT

Created attachment 100627 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Ryosuke Niwa

Comment 10 2011-07-12 23:20:32 PDT

Comment on attachment 100621 [details] Patch 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.

Ryosuke Niwa

Comment 11 2011-07-12 23:30:21 PDT

Comment on attachment 100621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100621&action=review >> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:60 >> + 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() + '"'); Oops, I misunderstood the logic here. You can probably do: var action = description + " in " + testId; var actualDirection = sel.baseOffset < sel.extentOffset ? 'forward' : (sel.baseOffset > sel.extentOffset ? 'backward' : 'none'); if (expectedDirection != actualDirection) testFailed(action + ' expected ' + direction + ' direction but got ' + actualDirection); else if (selection.toString() != expectedText) testFailed(action + expected "' + expectation + '" but got "' + selection.toString() + '"'); else testPassed(action);

Wyatt Carss

Comment 12 2011-07-13 20:21:49 PDT

Created attachment 100760 [details] Patch

Wyatt Carss

Comment 13 2011-07-13 20:30:15 PDT

Comment on attachment 100760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100760&action=review > LayoutTests/ChangeLog:10 > + text-input fields, and textareas. I still intend to add undo tests, but as said previously, I'm waiting until these are in a good state. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:67 > + runTestAndVerify(currentNode, makeTest('left', 'line'), macIsBackwardWinAndUnixAreForward, expectedResult[currentPlatform]); This seemed unwieldy to shove into a single long line, but I'm not sure if it would be preferred that way. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:76 > + runTestsOn(platform, document.getElementById('editable-div')); I started off with a node-travesal like so: for(i = 0; i < 2; i++) runTestsOn(platform, document.getElementById('test').children[i]; but it gave me a wacky syntax error the second time through, complaining about '1' as a property to undefined. I messed with it for 15 minutes and opted for explicitly showing the two tags instead. It's the same number of lines and similarly clear what's going on, but if you'd like me to look into the for loop solution, I'll try harder at it :P > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-2-expected.txt:14 > +PASS right by line in text-input is this sufficient output? I feel like I'm not saying enough..

Ryosuke Niwa

Comment 14 2011-07-13 21:12:22 PDT

Comment on attachment 100760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100760&action=review I don't think we need to split the test into two files. If you're doing that, then you should probably share the test code by adding some js file into resources. If not, then you can branch into two code paths by checking node.setSelectionRange because that's defined iff the node was input or textarea. Alternatively, you can just check node.localName == 'div' since you're only using div in the other cases. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:13 > + <div id="regular-div">Superfluous line.<br>Superfluous line.<br>The first sentence. The second sentence. Three sentences.<br>Another sentence. A fifth sentence. A sixth sentence.<br>Sentence seven. Sentence eight. The last sentence.<br>Superfluous line.<br>Superfluous line.</div> Can we just have 3 lines instead? All these extra lines and sentences are making this code less readable. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:50 > +function makeTest(direction, granularity) { I'd call this makeSelectionModifier. >> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:76 >> + runTestsOn(platform, document.getElementById('editable-div')); > > I started off with a node-travesal like so: > > for(i = 0; i < 2; i++) > runTestsOn(platform, document.getElementById('test').children[i]; > > but it gave me a wacky syntax error the second time through, complaining about '1' as a property to undefined. I messed with it for 15 minutes and opted for explicitly showing the two tags instead. It's the same number of lines and similarly clear what's going on, but if you'd like me to look into the for loop solution, I'll try harder at it :P It's childNodes, not children! You should also skip text nodes generated by whitespace around tags. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-2.html:61 > + expectedResult = {'mac': 'Superfluous line. Superfluous line. The first sentence. The second sentence. Three sentences. Another sentence. A fifth', If we're special-casing input element then why do we need so much text in the first place? It seems like we can trim it to be 1-2 words.

Wyatt Carss

Comment 15 2011-07-14 09:31:54 PDT

Comment on attachment 100760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100760&action=review >> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:13 >> + <div id="regular-div">Superfluous line.<br>Superfluous line.<br>The first sentence. The second sentence. Three sentences.<br>Another sentence. A fifth sentence. A sixth sentence.<br>Sentence seven. Sentence eight. The last sentence.<br>Superfluous line.<br>Superfluous line.</div> > > Can we just have 3 lines instead? All these extra lines and sentences are making this code less readable. At this point, I likely can -- I had to add them for obscure reasons earlier, so this should work fine. >>> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-1.html:76 >>> + runTestsOn(platform, document.getElementById('editable-div')); >> >> I started off with a node-travesal like so: >> >> for(i = 0; i < 2; i++) >> runTestsOn(platform, document.getElementById('test').children[i]; >> >> but it gave me a wacky syntax error the second time through, complaining about '1' as a property to undefined. I messed with it for 15 minutes and opted for explicitly showing the two tags instead. It's the same number of lines and similarly clear what's going on, but if you'd like me to look into the for loop solution, I'll try harder at it :P > > It's childNodes, not children! You should also skip text nodes generated by whitespace around tags. children is like childNodes, but doesn't have those whitespace nodes. >> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless-2.html:61 >> + expectedResult = {'mac': 'Superfluous line. Superfluous line. The first sentence. The second sentence. Three sentences. Another sentence. A fifth', > > If we're special-casing input element then why do we need so much text in the first place? It seems like we can trim it to be 1-2 words. good point!

WebKit Review Bot

Comment 16 2011-07-14 10:39:58 PDT

Comment on attachment 100760 [details] Patch Attachment 100760 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9061293 New failing tests: editing/deleting/delete-3608445-fix.html editing/deleting/delete-br-009.html editing/deleting/delete-br-005.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-br-007.html editing/deleting/delete-and-undo.html editing/execCommand/paste-2.html editing/deleting/delete-3608462-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-4083333-fix.html editing/execCommand/find-after-replace.html editing/execCommand/paste-1.html editing/deleting/delete-at-start-or-end.html editing/deleting/delete-br-006.html editing/deleting/delete-br-010.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/execCommand/format-block.html editing/deleting/delete-br-002.html editing/deleting/delete-character-001.html editing/deleting/delete-br-004.html

WebKit Review Bot

Comment 17 2011-07-14 10:40:02 PDT

Created attachment 100828 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Ryosuke Niwa

Comment 18 2011-07-14 10:43:37 PDT

By the way, EWS bots are pointing out that your patch breaks the following tests: editing/deleting/delete-3608445-fix.html editing/deleting/delete-br-009.html editing/deleting/delete-br-005.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-br-007.html editing/deleting/delete-and-undo.html editing/execCommand/paste-2.html editing/deleting/delete-3608462-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-4083333-fix.html editing/execCommand/find-after-replace.html editing/execCommand/paste-1.html editing/deleting/delete-at-start-or-end.html editing/deleting/delete-br-006.html editing/deleting/delete-br-010.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/execCommand/format-block.html editing/deleting/delete-br-002.html editing/deleting/delete-character-001.html editing/deleting/delete-br-004.html YOu should investigate why and fix that.

Wyatt Carss

Comment 19 2011-07-14 10:59:05 PDT

(In reply to comment #18) > By the way, EWS bots are pointing out that your patch breaks the following tests: > editing/deleting/delete-3608445-fix.html > editing/deleting/delete-br-009.html > editing/deleting/delete-br-005.html > editing/deleting/delete-3800834-fix.html > editing/deleting/delete-br-007.html > editing/deleting/delete-and-undo.html > editing/execCommand/paste-2.html > editing/deleting/delete-3608462-fix.html > editing/deleting/collapse-whitespace-3587601-fix.html > editing/deleting/delete-4083333-fix.html > editing/execCommand/find-after-replace.html > editing/execCommand/paste-1.html > editing/deleting/delete-at-start-or-end.html > editing/deleting/delete-br-006.html > editing/deleting/delete-br-010.html > editing/deleting/delete-at-paragraph-boundaries-008.html > editing/execCommand/format-block.html > editing/deleting/delete-br-002.html > editing/deleting/delete-character-001.html > editing/deleting/delete-br-004.html > > YOu should investigate why and fix that. Yeah.. my plan is to finish the small updates, write the undo test, then see what I can do to fix the test breakages. In general, they are a small editing delegate change which I haven't really grokked yet, but there's 1 or 2 image differences floating around in there too. It's particularly weird that it's turning up on cr-linux.

Wyatt Carss

Comment 20 2011-07-14 15:06:59 PDT

Created attachment 100869 [details] Patch

Ryosuke Niwa

Comment 21 2011-07-14 15:15:59 PDT

Comment on attachment 100869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100869&action=review > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:14 > + The first sentence. The second sentence. Three sentences.<br> We can be more aggressive and have 1-3 words in each line. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:79 > + macIsBackwardWinAndUnixAreForward = {'mac': 'backward', 'win': 'forward', 'unix': 'forward'}[currentPlatform]; This variable name repeats what RHS says. I want to know what this variable is for instead. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:81 > + allPlatformsAreForward = 'forward'; > + allPlatformsAreBackward = 'backward'; Ditto. Also, I don't see any benefit in having these variables around. It only bloats the code as far as I can tell. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:100 > + expectedResult = {'mac': 'e second sentence. Three sentences.\nAnother sentence. A test', > + 'win': 'econd sentence. Three sentences.\nAnother sentence. A ', > + 'unix': 'econd sentence. Three sentences.\nAnother sentence. A '}; I've got a feeling that these results are platform dependent because it depends on the width of words. You should really make lines MUCH shorter (probably just one or two words) and set the selection at the beginning of line instead of in the middle of it. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:120 > + if(listOfTestNodes[i].nodeName != '#text') Space after for and if. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:121 > + for(i = 0; i < listOfTestNodes.length; i++) > + if(listOfTestNodes[i].nodeName != '#text') > + runTestsOn(platform, listOfTestNodes[i]); You need { } around the if-statement; i.e. for (i = 0; i < listOfTestNodes.length; i++) { if (listOfTestNodes[i].nodeName != '#text') runTestsOn(platform, listOfTestNodes[i]); }

Wyatt Carss

Comment 22 2011-07-14 16:41:03 PDT

Comment on attachment 100869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100869&action=review >> LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:81 >> + allPlatformsAreBackward = 'backward'; > > Ditto. Also, I don't see any benefit in having these variables around. It only bloats the code as far as I can tell. Using the full object notation on every line makes the lines very hard to read. these variables seemed like a good way to compress the size and increase readability. Perhaps I could try "expectForwardOnAllPlatforms" and "expectBackwardOnMacAndForwardOnUnixAndWin"?

WebKit Review Bot

Comment 23 2011-07-14 16:43:25 PDT

Comment on attachment 100869 [details] Patch Attachment 100869 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9064384 New failing tests: editing/deleting/delete-3608445-fix.html editing/deleting/delete-br-009.html editing/deleting/delete-br-005.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-br-007.html editing/deleting/delete-and-undo.html editing/execCommand/paste-2.html editing/deleting/delete-3608462-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-4083333-fix.html editing/execCommand/find-after-replace.html editing/execCommand/paste-1.html editing/deleting/delete-at-start-or-end.html editing/deleting/delete-br-006.html editing/deleting/delete-br-010.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/execCommand/format-block.html editing/deleting/delete-br-002.html editing/deleting/delete-character-001.html editing/deleting/delete-br-004.html

WebKit Review Bot

Comment 24 2011-07-14 16:43:30 PDT

Created attachment 100895 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Wyatt Carss

Comment 25 2011-07-22 18:48:49 PDT

Created attachment 101801 [details] Patch

Ryosuke Niwa

Comment 26 2011-07-22 19:18:41 PDT

Comment on attachment 101801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101801&action=review mn... it's really hard to tell when we should be preserving isDirectional here. On one hand, it seems odd to preserve isDirectional if we were creating selection out of a position and new selection is nothing to do with the old selection but on the other hand, in some cases, new selection is visually identical to old selection to users and user would expect base and extent to persist over editing operations. Even more, setting extent or base will have a different effect depending on whether the caret selection was directionless or not. Let me think about this issue for a while. Meanwhile, other reviewers are welcome to review or comment on this bug. > Source/WebCore/editing/ApplyBlockElementCommand.cpp:114 > - setEndingSelection(VisibleSelection(positionBeforeNode(placeholder.get()), DOWNSTREAM)); > + setEndingSelection(VisibleSelection(positionBeforeNode(placeholder.get()), DOWNSTREAM, endingSelection().isDirectional())); It's not clear that we should be preserving isDirectional of the original endingSelection here. > Source/WebCore/editing/BreakBlockquoteCommand.cpp:81 > - setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM)); > + setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional())); Ditto about preserving isDirectional here. > Source/WebCore/editing/BreakBlockquoteCommand.cpp:91 > - setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM)); > + setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/BreakBlockquoteCommand.cpp:124 > - setEndingSelection(VisibleSelection(VisiblePosition(firstPositionInOrBeforeNode(startNode)))); > + setEndingSelection(VisibleSelection(VisiblePosition(firstPositionInOrBeforeNode(startNode)), endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/BreakBlockquoteCommand.cpp:197 > - setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM)); > + setEndingSelection(VisibleSelection(positionBeforeNode(breakNode.get()), DOWNSTREAM, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/CompositeEditCommand.cpp:913 > - setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > + setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional())); I don't think we should be using endingSelection's isDirectional here because this a temporary selection used by deleteSelection to delete the contents we're moving. > Source/WebCore/editing/CompositeEditCommand.cpp:1014 > - setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > + setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/CompositeEditCommand.cpp:1123 > - setEndingSelection(VisibleSelection(firstPositionInNode(newBlock.get()), DOWNSTREAM)); > + setEndingSelection(VisibleSelection(firstPositionInNode(newBlock.get()), DOWNSTREAM, endingSelection().isDirectional())); Ditto about setting isDirectional on a position. > Source/WebCore/editing/CompositeEditCommand.cpp:1161 > - setEndingSelection(VisibleSelection(atBR)); > + setEndingSelection(VisibleSelection(atBR, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/DeleteSelectionCommand.cpp:824 > - setEndingSelection(VisibleSelection(m_endingPosition, affinity)); > + setEndingSelection(VisibleSelection(m_endingPosition, affinity, endingSelection().isDirectional())); Ditto about setting isDirectional on caret. > Source/WebCore/editing/InsertLineBreakCommand.cpp:124 > - setEndingSelection(VisibleSelection(endingPosition)); > + setEndingSelection(VisibleSelection(endingPosition, endingSelection().isDirectional())); Ditto.

WebKit Review Bot

Comment 27 2011-07-22 19:18:48 PDT

Comment on attachment 101801 [details] Patch Attachment 101801 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9226347 New failing tests: editing/pasteboard/smart-paste-001.html editing/pasteboard/subframe-dragndrop-1.html editing/pasteboard/4631972.html editing/pasteboard/smart-paste-007.html editing/pasteboard/smart-drag-drop.html editing/pasteboard/drag-drop-dead-frame.html editing/pasteboard/smart-paste-008.html editing/execCommand/find-after-replace.html editing/deleting/smart-delete-002.html editing/inserting/insert-text-with-newlines.html editing/execCommand/paste-2.html editing/pasteboard/smart-paste-003.html editing/pasteboard/smart-paste-004.html editing/pasteboard/copy-standalone-image.html editing/selection/anchor-focus2.html editing/pasteboard/interchange-newline-1.html editing/deleting/smart-delete-001.html editing/selection/after-line-break.html editing/execCommand/paste-1.html editing/selection/4776665.html

Wyatt Carss

Comment 28 2011-07-26 17:40:48 PDT

After the last patch, I've made a spreadsheet to record my progress and reasoning on whether or not to preserve isDirectionality at each call to VisibleSelection's constructors. It is here: https://spreadsheets.google.com/spreadsheet/ccc?key=0AhuT3gDPvJi6dGgxSzJnelFBMXg4cUE3VkxRVTVzdFE&hl=en_US The gist of the chart is: should this call preserve isDirectionality, why, and then info about the call (which is shown as it was written in my most recent patch here) After going through all of the constructor uses and removing the isDirectionality preservation where it did not seem to make sense, several bugs appeared, so it is clear that my reasoning for not preserving isDirectionality is flawed in at least some cases. Feel free to change the font on the spreadsheet; courier-new seemed to be the most readable, which isn't saying much :P

Levi Weintraub

Comment 29 2011-07-26 17:54:21 PDT

I'm happy to take a look at the spreadsheet when I have an idea which tests are failing :)

Wyatt Carss

Comment 30 2011-07-26 18:03:24 PDT

good suggestion! the tests that regressed after making the changes in the spreadsheet would be useful: editing/undo/5378473.html editing/undo/replace-text-in-node-preserving-markers-crash.html editing/undo/undo-combined-delete-boundary.html editing/undo/undo-combined-delete.html editing/undo/undo-delete-boundary.html editing/undo/undo-delete.html editing/undo/undo-forward-delete-boundary.html editing/undo/undo-forward-delete.html editing/undo/undo-misspellings.html

Wyatt Carss

Comment 31 2011-07-29 14:36:52 PDT

Created attachment 102399 [details] Patch

Wyatt Carss

Comment 32 2011-07-29 14:40:41 PDT

Comment on attachment 102399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102399&action=review > Source/WebCore/ChangeLog:16 > + This is really just the "minimum possible" patch, which should pass tests and work as expected. I'm expecting Ryosuke to find some spots where we should preserve isDirectionality, and I'm also expecting to find some by writing some additional undo tests that are explicitly related to directionality of selections. Then I'll make those fixes and upload those tests, and this will hopefully be set to commit :)

Wyatt Carss

Comment 33 2011-08-01 14:15:29 PDT

Created attachment 102555 [details] Patch

Ryosuke Niwa

Comment 34 2011-08-01 14:16:54 PDT

Comment on attachment 102399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102399&action=review r- due to various nits. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:41 > + range.setStart(container, container.data.search('test')); > + range.setEnd(container, range.startOffset + 'test'.length); Since you're selecting the first appearance of "test" instead of the forth word, it might be better to call this function selectTestInDiv. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:46 > +function selectFourthWordInSecondLineOfTextField(container) { Ditto about the function name. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:79 > + macIsBackwardWinAndUnixAreForward = {'mac': 'backward', 'win': 'forward', 'unix': 'forward'}[currentPlatform]; I still think this variable name isn't describe. It should be backwardOnMacAndForwardOtherwise or backwardOnMacAndForwardOnWinAndUnix. Also, you're missing "var". > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:81 > + allPlatformsAreBackward = 'backward'; It appears that this variable is never used. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:86 > + extendSelectionLeftByCharacter = makeSelectionModifier('left', 'character'); > + extendSelectionRightByCharacter = makeSelectionModifier('right', 'character'); > + extendSelectionLeftByLine = makeSelectionModifier('left', 'line'); > + extendSelectionRightByLine = makeSelectionModifier('right', 'line'); Missing var :( > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:92 > + expectedResult = {'mac': 'The test', 'win': 'The ', 'unix': 'The '}; Missing "var" again. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:96 > + 'win': 'second sentence. Three sentences.\nAnother sentence. A ', > + 'unix': 'second sentence. Three sentences.\nAnother sentence. A '}; Nit: we don't deep-indent to align lines. This line should be intended by 4 spaces in addition to the indentation before the previous line. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:100 > + 'win': 'econd sentence. Three sentences.\nAnother sentence. A ', > + 'unix': 'econd sentence. Three sentences.\nAnother sentence. A '}; Ditto about the deep indentation. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:111 > + runTestAndVerify(currentNode, extendSelectionRightByLine, allPlatformsAreForward, expectedResult); Just pass 'forward' here. > LayoutTests/platform/chromium-win/editing/deleting/delete-at-start-or-end-expected.txt:4 > -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification > +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE It's kind of odd that we don't have webViewDidChangeSelection after calling shouldChangeSelectedDOMRange. It's not like the editor client is canceling it. > Source/WebCore/ChangeLog:9 > + to 'false', and added a parameter to one VisibleSelection constructor I don't think the term 'parameter' is used in C++ to mean an argument. > Source/WebCore/ChangeLog:12 > + Also modified endingSelection() in two spots to preserve isDirectionality s/isDirectionality/isDirectional/. > Source/WebCore/ChangeLog:15 > + Many more spots likely exist where preserving directionality is a good > + idea, and some of those will be found by forthcoming undo tests. Are we adding new undo tests? If so, where? We should file a bug and put the bug number here. On the other hand, if we're not doing this, then this comment seems redundant. > Source/WebCore/editing/VisibleSelection.h:45 > + VisibleSelection(const Position&, EAffinity, bool = false); We should probably say what this new variable is because "bool = false" doesn't tell us any semantics.

Wyatt Carss

Comment 35 2011-08-01 14:17:55 PDT

Comment on attachment 102555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102555&action=review > LayoutTests/ChangeLog:22 > + * editing/selection/programmatic-selection-on-mac-is-directionless.html: Added. These now test undo after insertText, delete, forwardDelete, bold, and cut (and they all pass!) My earlier thoughts that other forms of undo would be found to be broken aren't panning out; the endingSelection() modifications seem to be pretty far reaching.

WebKit Review Bot

Comment 36 2011-08-01 14:55:20 PDT

Comment on attachment 102555 [details] Patch Attachment 102555 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9283680 New failing tests: fast/forms/selection-direction.html editing/selection/programmatic-selection-on-mac-is-directionless.html

Wyatt Carss

Comment 37 2011-08-01 16:28:34 PDT

Created attachment 102589 [details] Patch

Wyatt Carss

Comment 38 2011-08-01 16:30:10 PDT

Comment on attachment 102589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102589&action=review > LayoutTests/ChangeLog:11 > + fast/forms/selection-direction.html still fails due to a direction not being preserved when focus is lost. I'm debugging that at the moment, but I wanted to put this up for feedback now that the undo tests are written.

WebKit Review Bot

Comment 39 2011-08-01 17:24:25 PDT

Comment on attachment 102589 [details] Patch Attachment 102589 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9284575 New failing tests: fast/forms/selection-direction.html

Wyatt Carss

Comment 40 2011-08-02 16:27:23 PDT

Created attachment 102711 [details] Patch

Wyatt Carss

Comment 41 2011-08-02 20:59:04 PDT

Created attachment 102736 [details] Patch

Wyatt Carss

Comment 42 2011-08-02 21:04:28 PDT

The latest patch passes tests and has had to have significantly fewer tests rebaselined than the original minimal-patch did. I've modified the spreadsheet with the present state (farthest left column) and a reasoning any time there wasn't one listed to the right, or that the one listed to the right needed to be updated. I've only gone back through the cases where rniwa suggested preservation was a good idea; I will look again at the rest tomorrow, and see how they impact the tests - but I'm expecting that this is pretty close to a final patch.

Ryosuke Niwa

Comment 43 2011-08-02 21:24:07 PDT

Comment on attachment 102736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102736&action=review The patch looks much better. Hopefully most of these rebaselines will go away as you preserve / set isDirectional more. > Source/WebCore/editing/FrameSelection.cpp:1029 > + We don't need a blank line here. > Source/WebCore/editing/FrameSelection.cpp:1030 > + setSelection(VisibleSelection(pos.deepEquivalent(), m_selection.extent(), pos.affinity(), selectionHasDirection), CloseTyping | ClearTypingStyle | userTriggered); Huh, CloseTyping | ClearTypingStyle is repeated everywhere (there are two places in FrameSelection.h as well). Can we define SetSelectionDefaultOptions and use that instead? That could be a separate patch.

Wyatt Carss

Comment 44 2011-08-04 15:18:40 PDT

Created attachment 102989 [details] Patch

Wyatt Carss

Comment 45 2011-08-04 15:35:30 PDT

Comment on attachment 102989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102989&action=review The spreadsheet linked above has a list of all of the spots where directionality is preserved, on the far left, with justification included where reasonable. > LayoutTests/platform/chromium-linux/editing/pasteboard/subframe-dragndrop-1-expected.txt:2 > +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification These rebaselines seem to be from the no-argument VisibleSelection constructor; when a frame is constructed, it makes an empty frame selection which has an empty visible selection, which is directionless. It then (pretty immediately) calls setSelection, in a spot where we make a new selection which is optionally directional or not depending on the platform. If the new selection is directional, and the old one isn't, this delegate occurs. I think that the best solution would be to somehow have the no-arg VisibleSelection constructor (and all others, for that matter) immediately take on the correct value for isDirectional, but VisibleSelection only has access to the editingBehavior through pos->document->frame->behavior (which by the way, uses frame->settings to find the behavior, which seems silly), and frame isn't guaranteed to be accessible. So, this method presently causes a bunch of crashes and unhappiness, and by comparison, 11 extra delegate lines seem alright.

Ryosuke Niwa

Comment 46 2011-08-04 15:47:30 PDT

(In reply to comment #45) > I think that the best solution would be to somehow have the no-arg VisibleSelection constructor (and all others, for that matter) immediately take on the correct value for isDirectional, but VisibleSelection only has access to the editingBehavior through pos->document->frame->behavior (which by the way, uses frame->settings to find the behavior, which seems silly), and frame isn't guaranteed to be accessible. So, this method presently causes a bunch of crashes and unhappiness, and by comparison, 11 extra delegate lines seem alright. Alternatively, can we set isDirectional(true) in FrameSelection's constructor?

Wyatt Carss

Comment 47 2011-08-04 16:01:53 PDT

(In reply to comment #46) > (In reply to comment #45) > > I think that the best solution would be to somehow have the no-arg VisibleSelection constructor (and all others, for that matter) immediately take on the correct value for isDirectional, but VisibleSelection only has access to the editingBehavior through pos->document->frame->behavior (which by the way, uses frame->settings to find the behavior, which seems silly), and frame isn't guaranteed to be accessible. So, this method presently causes a bunch of crashes and unhappiness, and by comparison, 11 extra delegate lines seem alright. > > Alternatively, can we set isDirectional(true) in FrameSelection's constructor? Do you mean VisibleSelection? I think that that would cause the inverse issue on macs; what is presently directional would be non-directional, and vice versa, in this specific case.

Ryosuke Niwa

Comment 48 2011-08-04 16:51:32 PDT

(In reply to comment #47) > (In reply to comment #46) > > Alternatively, can we set isDirectional(true) in FrameSelection's constructor? > > Do you mean VisibleSelection? I think that that would cause the inverse issue on macs; what is presently directional would be non-directional, and vice versa, in this specific case. No. In FrameSelection::FrameSelection, you can call shouldAlwaysUseDirectionalSelection because you have access to frame there. So you'd do: if (shouldAlwaysUseDirectionalSelection(m_frame)) m_selection.setIsDirectional(true); (of course, you'd have to move the definition of shouldAlwaysUseDirectionalSelection to the top of the file).

Ryosuke Niwa

Comment 49 2011-08-04 16:54:05 PDT

Also, this patch is failing to build on Mac because you didn't update WebCore.exp.in: "__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_", referenced from: -exported_symbol[s_list] command line option (maybe you meant: __ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_b) "__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE", referenced from: -exported_symbol[s_list] command line option The fix as simple as replacing __ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_ and __ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE by new mangled names. Use nm to find out the mangled names of these functions.

Wyatt Carss

Comment 50 2011-08-04 17:45:01 PDT

Created attachment 103011 [details] Patch

Ryosuke Niwa

Comment 51 2011-08-04 17:46:23 PDT

So adding setIsDirectional didn't help in reducing the rebaselines?

Wyatt Carss

Comment 52 2011-08-04 17:52:03 PDT

Created attachment 103012 [details] Patch

Wyatt Carss

Comment 53 2011-08-04 17:53:19 PDT

I'm not sure if the changes to WebCore.exp.in made it in.. they didn't show up in the changelog. That approach worked though, it appears that no tests need to be rebaselined. Because I'm in a terrible rush, I haven't run local tests on this yet -- I will as soon as possible, but I'm ooo tomorrow and monday.

Ryosuke Niwa

Comment 54 2011-08-04 18:49:11 PDT

Comment on attachment 103012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103012&action=review > Source/WebCore/WebCore.exp.in:490 > -__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE > -__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_ > +__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityES3_b > +__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionEb You need: -__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE -__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_ +__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityEb +__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionEb

WebKit Review Bot

Comment 55 2011-08-05 18:28:01 PDT

Comment on attachment 103012 [details] Patch Attachment 103012 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9327038 New failing tests: editing/pasteboard/subframe-dragndrop-1.html fast/selectors/unqualified-hover-strict.html fast/forms/implicit-submission.html editing/pasteboard/4631972.html editing/selection/select-all-iframe.html editing/execCommand/paste-2.html editing/pasteboard/copy-standalone-image.html editing/execCommand/find-after-replace.html editing/selection/iframe.html fast/forms/input-spinbutton-capturing.html fast/forms/input-number-large-padding.html editing/selection/drag-to-contenteditable-iframe.html editing/selection/4776665.html fast/forms/input-number-events.html editing/pasteboard/drag-drop-dead-frame.html editing/execCommand/paste-1.html

Wyatt Carss

Comment 56 2011-08-10 12:40:23 PDT

Created attachment 103518 [details] Patch

Ryosuke Niwa

Comment 57 2011-08-10 15:55:21 PDT

Comment on attachment 103518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103518&action=review The patch looks great! r- due to a bug in the test but I think we're almost ready to land this patch. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:53 > +function runTestsAndVerify(currentNode, currentSelectionModifier, expectedDirection, expectedText) { > + var currentTest = makeTest(currentNode, currentSelectionModifier, expectedDirection, expectedText); Nit: It seems odd to call arguments "current" since there's only one node touched inside this function. I suggest rename these two arguments to be name and selectionModifier. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:63 > +function makeTest(currentNode, runCurrentTest, expectedDirection, expectedText) { Nit: Ditto about currentNode. Also, why are we calling the second argument as runCurrentTest? That's a very general name that can mean almost anything since the entire file is a test. Please call it selectionModifier as done elsewhere. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:72 > + if (action == 'insertText' || action == 'forwardDelete') > + window.getSelection().modify(action, 'word'); modify only accepts "move" or "extend" for the first argument. r- because of this. You probably meant execCommand(action, false, 'word'); But since it's always safe to pass the 3rd argument, I'd suggest you always call document.execCommand(action, false, 'word') instead. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:85 > +function selectTestInElement(currentNode) { I'd move this function above runTestsAndVerify so that all 3 functions are put together. Also I'd rename currentNode to node. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:94 > +function verifyResult(expectedDirection, expectedText, description) { This function is only called in makeTest, and I'd rather see all of code inside makeTest so that I can grasp the idea of what has been tested there. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:114 > +function runTestsOn(currentPlatform, currentNode) { Nit: ditto about "current" in currentPlatform and currentNode. > LayoutTests/editing/selection/programmatic-selection-on-mac-is-directionless.html:115 > + var backwardOnMacAndForwardOtherwise = {'mac': 'backward', 'win': 'forward', 'unix': 'forward'}[currentPlatform]; Since win and unix will always have the same results in all these tests, I'd define a local variable like macOrNonMac = platform == 'mac' ? 'non-mac'; to reduce the duplication. > Source/WebCore/ChangeLog:10 > + to make programmatic selection be directionless by default on mac. s/mac/Mac/ > Source/WebCore/ChangeLog:12 > + Also modified endingSelection() in two spots to preserve isDirectionality Could you tells us where they were?

Ryosuke Niwa

Comment 58 2011-08-10 16:03:08 PDT

Comment on attachment 103518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103518&action=review > Source/WebCore/dom/Element.cpp:1677 > - VisibleSelection newSelection = VisibleSelection(firstPositionInOrBeforeNode(this), DOWNSTREAM); > + VisibleSelection newSelection = VisibleSelection(firstPositionInOrBeforeNode(this), DOWNSTREAM, frame->selection()->selection().isDirectional()); I don't think we should be preserving isDirectional here because we're creating new selection. Is there any test that proves otherwise?

Ryosuke Niwa

Comment 59 2011-08-10 16:27:11 PDT

Comment on attachment 103518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103518&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:909 > - setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > + setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional())); I don't think it makes any sense for us to preserve ending selection's direction here. If anything, we should be passing true because start and end are literally nothing to do with endingSelection. > Source/WebCore/editing/CompositeEditCommand.cpp:1010 > - setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > + setEndingSelection(VisibleSelection(start, end, DOWNSTREAM, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/CompositeEditCommand.cpp:1037 > - setEndingSelection(destination); > + setEndingSelection(VisibleSelection(destination, endingSelection().isDirectional())); Ditto. > Source/WebCore/editing/FrameSelection.cpp:859 > + m_selection.setIsDirectional(shouldAlwaysUseDirectionalSelection(m_frame) || alter == AlterationExtend); I'd like to see the rationale for moving this statement in the change log.

Ryosuke Niwa

Comment 60 2011-08-10 16:47:32 PDT

Comment on attachment 103518 [details] Patch How about line 87 and 92 in InsertTextCommand.cpp inside performTrivialReplace? It appears that we should preserve isDirectional in there as well. Also, you're missing two instances in BreakBlockquoteCommand.cpp.

Ryosuke Niwa

Comment 61 2011-08-10 16:47:35 PDT

Comment on attachment 103518 [details] Patch How about line 87 and 92 in InsertTextCommand.cpp inside performTrivialReplace? It appears that we should preserve isDirectional in there as well. Also, you're missing two instances in BreakBlockquoteCommand.cpp.

Ryosuke Niwa

Comment 62 2011-08-10 16:59:59 PDT

Mac build fix: -__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE -__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_ +__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityEb +__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionEb +__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_b

Ryosuke Niwa

Comment 63 2011-08-10 18:15:07 PDT

Created attachment 103565 [details] test failures on mac The current patch fails on Mac, and there's the diff I'm seeing.

Wyatt Carss

Comment 64 2011-08-11 16:20:53 PDT

Created attachment 103698 [details] Patch

Wyatt Carss

Comment 65 2011-08-11 16:45:44 PDT

Comment on attachment 103698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103698&action=review > LayoutTests/ChangeLog:11 > + There are 3 tests to baseline on cr-linux at the moment

WebKit Review Bot

Comment 66 2011-08-11 17:06:25 PDT

Comment on attachment 103698 [details] Patch Attachment 103698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9358404 New failing tests: editing/deleting/whitespace-pre-1.html editing/deleting/delete-ligature-003.html editing/deleting/paragraph-in-preserveNewline.html

Wyatt Carss

Comment 67 2011-08-11 18:34:21 PDT

Created attachment 103718 [details] Patch

Wyatt Carss

Comment 68 2011-08-11 18:35:42 PDT

Comment on attachment 103718 [details] Patch Those three tests have been rebaselined to be platform independent.

Wyatt Carss

Comment 69 2011-08-12 15:49:01 PDT

Created attachment 103832 [details] Patch

Wyatt Carss

Comment 70 2011-08-12 15:50:48 PDT

Comment on attachment 103832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103832&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1037 > + setEndingSelection(VisibleSelection(destination, document()->frame()->editor()->behavior().shouldConsiderSelectionAsDirectional())); This one did end up being necessary for LayoutTests/editing/pasteboard/interchange-newline-1.html to work.

Wyatt Carss

Comment 71 2011-08-12 17:05:20 PDT

Created attachment 103845 [details] Patch

Wyatt Carss

Comment 72 2011-08-12 17:07:26 PDT

Fixed changelogs

Wyatt Carss

Comment 73 2011-08-12 18:01:42 PDT

Created attachment 103849 [details] Patch

Ryosuke Niwa

Comment 74 2011-08-12 18:14:39 PDT

Comment on attachment 103849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103849&action=review I'm going to land this patch on Tuesday on behalf of Wyatt since he's not a committer yet and this is a really large patch. > LayoutTests/ChangeLog:12 > + Reviewed by NOBODY (OOPS!). This line should be before the description. > Source/WebCore/ChangeLog:18 > + Reviewed by NOBODY (OOPS!). Ditto.

Ryosuke Niwa

Comment 75 2011-08-16 11:40:01 PDT

Committed r93134: <http://trac.webkit.org/changeset/93134>

Attachments
Patch (11.35 KB, patch) 2011-07-11 12:21 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (16.82 KB, patch) 2011-07-12 21:23 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.79 MB, application/zip) 2011-07-12 21:52 PDT, WebKit Review Bot	no flags	Details
Patch (16.13 KB, patch) 2011-07-13 20:21 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (283.18 KB, application/zip) 2011-07-14 10:40 PDT, WebKit Review Bot	no flags	Details
Patch (12.25 KB, patch) 2011-07-14 15:06 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (2.24 MB, application/zip) 2011-07-14 16:43 PDT, WebKit Review Bot	no flags	Details
Patch (39.92 KB, patch) 2011-07-22 18:48 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (138.84 KB, patch) 2011-07-29 14:36 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (153.80 KB, patch) 2011-08-01 14:15 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (153.69 KB, patch) 2011-08-01 16:28 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (159.40 KB, patch) 2011-08-02 16:27 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (77.00 KB, patch) 2011-08-02 20:59 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (74.74 KB, patch) 2011-08-04 15:18 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (76.30 KB, patch) 2011-08-04 17:45 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (75.42 KB, patch) 2011-08-04 17:52 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (64.38 KB, patch) 2011-08-10 12:40 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
test failures on mac (88.03 KB, patch) 2011-08-10 18:15 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch (126.29 KB, patch) 2011-08-11 16:20 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (148.79 KB, patch) 2011-08-11 18:34 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (147.67 KB, patch) 2011-08-12 15:49 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (148.11 KB, patch) 2011-08-12 17:05 PDT, Wyatt Carss	no flags	Details Formatted Diff Diff
Patch (148.68 KB, patch) 2011-08-12 18:01 PDT, Wyatt Carss	rniwa: review+	Details Formatted Diff Diff
Show Obsolete (21) View All Add attachment proposed patch, testcase, etc.