Bug 60529 - Programmatically set selection should not have direction on Mac
Summary: Programmatically set selection should not have direction on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 60403 63473
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 20:50 PDT by Ryosuke Niwa
Modified: 2011-08-16 11:40 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Justin Garcia 2011-05-09 20:56:51 PDT
<rdar://problem/9411324>
Comment 2 Ryosuke Niwa 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.
Comment 3 Wyatt Carss 2011-07-11 12:21:06 PDT
Created attachment 100347 [details]
Patch
Comment 4 Ryosuke Niwa 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();
Comment 5 Ryosuke Niwa 2011-07-11 12:37:06 PDT
Comment on attachment 100347 [details]
Patch

r- due to inadequate tests and various nits.
Comment 6 Wyatt Carss 2011-07-12 21:23:50 PDT
Created attachment 100621 [details]
Patch
Comment 7 Wyatt Carss 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 :)
Comment 8 WebKit Review Bot 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
Comment 9 WebKit Review Bot 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
Comment 10 Ryosuke Niwa 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.
Comment 11 Ryosuke Niwa 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);
Comment 12 Wyatt Carss 2011-07-13 20:21:49 PDT
Created attachment 100760 [details]
Patch
Comment 13 Wyatt Carss 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..
Comment 14 Ryosuke Niwa 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.
Comment 15 Wyatt Carss 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!
Comment 16 WebKit Review Bot 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
Comment 17 WebKit Review Bot 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
Comment 18 Ryosuke Niwa 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.
Comment 19 Wyatt Carss 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.
Comment 20 Wyatt Carss 2011-07-14 15:06:59 PDT
Created attachment 100869 [details]
Patch
Comment 21 Ryosuke Niwa 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]);
}
Comment 22 Wyatt Carss 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"?
Comment 23 WebKit Review Bot 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
Comment 24 WebKit Review Bot 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
Comment 25 Wyatt Carss 2011-07-22 18:48:49 PDT
Created attachment 101801 [details]
Patch
Comment 26 Ryosuke Niwa 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.
Comment 27 WebKit Review Bot 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
Comment 28 Wyatt Carss 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
Comment 29 Levi Weintraub 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 :)
Comment 30 Wyatt Carss 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
Comment 31 Wyatt Carss 2011-07-29 14:36:52 PDT
Created attachment 102399 [details]
Patch
Comment 32 Wyatt Carss 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 :)
Comment 33 Wyatt Carss 2011-08-01 14:15:29 PDT
Created attachment 102555 [details]
Patch
Comment 34 Ryosuke Niwa 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.
Comment 35 Wyatt Carss 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.
Comment 36 WebKit Review Bot 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
Comment 37 Wyatt Carss 2011-08-01 16:28:34 PDT
Created attachment 102589 [details]
Patch
Comment 38 Wyatt Carss 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.
Comment 39 WebKit Review Bot 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
Comment 40 Wyatt Carss 2011-08-02 16:27:23 PDT
Created attachment 102711 [details]
Patch
Comment 41 Wyatt Carss 2011-08-02 20:59:04 PDT
Created attachment 102736 [details]
Patch
Comment 42 Wyatt Carss 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.
Comment 43 Ryosuke Niwa 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.
Comment 44 Wyatt Carss 2011-08-04 15:18:40 PDT
Created attachment 102989 [details]
Patch
Comment 45 Wyatt Carss 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.
Comment 46 Ryosuke Niwa 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?
Comment 47 Wyatt Carss 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.
Comment 48 Ryosuke Niwa 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).
Comment 49 Ryosuke Niwa 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.
Comment 50 Wyatt Carss 2011-08-04 17:45:01 PDT
Created attachment 103011 [details]
Patch
Comment 51 Ryosuke Niwa 2011-08-04 17:46:23 PDT
So adding setIsDirectional didn't help in reducing the rebaselines?
Comment 52 Wyatt Carss 2011-08-04 17:52:03 PDT
Created attachment 103012 [details]
Patch
Comment 53 Wyatt Carss 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.
Comment 54 Ryosuke Niwa 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
Comment 55 WebKit Review Bot 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
Comment 56 Wyatt Carss 2011-08-10 12:40:23 PDT
Created attachment 103518 [details]
Patch
Comment 57 Ryosuke Niwa 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?
Comment 58 Ryosuke Niwa 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?
Comment 59 Ryosuke Niwa 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.
Comment 60 Ryosuke Niwa 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.
Comment 61 Ryosuke Niwa 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.
Comment 62 Ryosuke Niwa 2011-08-10 16:59:59 PDT
Mac build fix:

-__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityE
-__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_
+__ZN7WebCore16VisibleSelectionC1EPKNS_5RangeENS_9EAffinityEb
+__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionEb
+__ZN7WebCore16VisibleSelectionC1ERKNS_15VisiblePositionES3_b
Comment 63 Ryosuke Niwa 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.
Comment 64 Wyatt Carss 2011-08-11 16:20:53 PDT
Created attachment 103698 [details]
Patch
Comment 65 Wyatt Carss 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
Comment 66 WebKit Review Bot 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
Comment 67 Wyatt Carss 2011-08-11 18:34:21 PDT
Created attachment 103718 [details]
Patch
Comment 68 Wyatt Carss 2011-08-11 18:35:42 PDT
Comment on attachment 103718 [details]
Patch

Those three tests have been rebaselined to be platform independent.
Comment 69 Wyatt Carss 2011-08-12 15:49:01 PDT
Created attachment 103832 [details]
Patch
Comment 70 Wyatt Carss 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.
Comment 71 Wyatt Carss 2011-08-12 17:05:20 PDT
Created attachment 103845 [details]
Patch
Comment 72 Wyatt Carss 2011-08-12 17:07:26 PDT
Fixed changelogs
Comment 73 Wyatt Carss 2011-08-12 18:01:42 PDT
Created attachment 103849 [details]
Patch
Comment 74 Ryosuke Niwa 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.
Comment 75 Ryosuke Niwa 2011-08-16 11:40:01 PDT
Committed r93134: <http://trac.webkit.org/changeset/93134>