Bug 25195 – mouse-based selections are always directional on Window/Linux

• Component: HTML Editing
• Status: RESOLVED FIXED

[Xiaomei Ji]

Steps to reproduce:
1. Open the attached test case.
2. place cursor after 'o' in "two".
3. drag mouse to left to select the word "two".
4. press shift+right arrow.

What is the current behavior:
selection extends to "two ", which includes the space between "two" and "three".

What is the behavior of FireFox and IE
selection became "wo", which un-selects the previous 't' from 'two'.

[Xiaomei Ji]

Created attachment 29483 [details]
test case

[Xiaomei Ji]

Same applies to the other direction of selection.

Steps to reproduce:
1. Open the attached test case.
2. place cursor before 't' in "two".
3. drag mouse to right to select the word "two".
4. press shift+left arrow.

What is the current behavior:
selection extends to " two", which includes the space between "one" and
"two".

What is the behavior of FireFox and IE
selection became "tw", which un-selects the previous 'o' from 'two'.

[Xiaomei Ji]

in chromium bug:
http://code.google.com/p/chromium/issues/detail?id=10487

[Xiaomei Ji]

The root cause seems is in SelectionController::willBeModified().

There are the following logic in SelectionController::willBeModified()
        case EXTEND:
            if (!m_lastChangeWasHorizontalExtension) {
                m_lastChangeWasHorizontalExtension = true;
                Position start = m_sel.start();
                Position end = m_sel.end();
                switch (direction) {
                    // FIXME: right for bidi?
                    case RIGHT:
                    case FORWARD:
                        m_sel.setBase(start);
                        m_sel.setExtent(end);
                        break;
                    case LEFT:
                    case BACKWARD:
                        m_sel.setBase(end);
                        m_sel.setExtent(start);
                        break;
                }
            }

If the previous selection is a mouse selection, m_lastChangeWasHorizontalExtension is 'false', and the selection's (base, extend) is reset.
For example, if the selection is "two" from 't' to 'o', (base, extend) and (start, end) were both (4, 7), but extend+left (by press shift+left arrow) reset  (base, extend) to (7, 4), then, the previous character of 'extend' is the space between "one" and "two". 
Similar applies to the other direction of selection.

'base' and 'extend' are supposed to be where the 'first' and the 'end' click happens. What is the rationale to reset it here?

It works fine for continuing selection using keyboard, because the m_lastChangeWasHorizontalExtension is 'true' for keyboard selection, and the reset never happens.

[Mark Rowe (bdash)]

The existing behavior matches the behavior of Mac OS X.

[Xiaomei Ji]

(In reply to comment #5)
> The existing behavior matches the behavior of Mac OS X.
> 

Thanks for the quick update.
Then, would it be OK if I provide a platform dependent patch to match the behavior or FF/IE in windows?

[Yair Yogev]

this issue is applicable to all mouse selections and not just editable ones
(in chromium bug http://code.google.com/p/chromium/issues/detail?id=4543 comment#5 )

[Xiaomei Ji]

FYI: how to write layout test to emulate a mouse selection event

From Mitz:

The way drag-selection works is that the base position of the selection is not decided on mouse down, but rather on the first mouse move (which is a mouse drag) after that. This is arguably a bug in EventHandler, and normally it doesn’t make a difference (because when a user drags, the first mouse move is very close to the mouse down). The way we work around this issue in layout tests is that we do another mouseMoveTo with the same coordinates after the mouseDown:

            eventSender.mouseMoveTo(x, y);
            eventSender.mouseDown();
            eventSender.mouseMoveTo(x, y);


==== My original question send to him =========================================================

I am trying to create a layout test for webkit issue 25195 patch.
In Windows platform, I commented out the m_sel's base/extent reset part mentioned in comment #4.

The code change makes difference. But I do not know how to emulate a mouse selection event in layout test.

I tried the following:

function positionsExtendingInDirection(sel, direction, granularity) {
            var positions = [];

            var start = document.getElementById("start");
            x = start.offsetLeft;
            y = start.offsetTop + 10;
            eventSender.mouseMoveTo(x, y);
            eventSender.mouseDown();
            positions.push({ node: sel.extentNode, begin: sel.baseOffset, end: sel.extentOffset });

            var end = document.getElementById("end");
            x = end.offsetLeft;
            y = end.offsetTop + 10;
            eventSender.mouseMoveTo(x, y);
            eventSender.mouseUp();
            positions.push({ node: sel.extentNode, begin: sel.baseOffset, end: sel.extentOffset });

            sel.modify("extend", direction, granularity);
            positions.push({ node: sel.extentNode, begin: sel.baseOffset, end: sel.extentOffset });
            return positions;
}
</script>
<body>
.....one <span id="start">two<span id="end"> three...
</body>


But such layout does not make any difference in Windows and Mac. Seems "mouseDown" + "mouseMoveTo" is not the same as mouse-drag. 

[Xiaomei Ji]

Created attachment 31969 [details]
patch w/ Layout test

[Darin Adler]

Comment on attachment 31969 [details]
patch w/ Layout test

Six thoughts:

    1) Please re-test this after http://trac.webkit.org/changeset/45945 -- this might have been affected by that bug.

    2) #if !PLATFORM(WIN) is the way to do this, not an #if followed by an #else.

    3) Does this really not affect the results of any other regression tests?

    4) We want the test to test behavior on all platforms, expecting one result on Windows and another on other platforms.

    5) Is it really true that Windows is the exception? Maybe Mac is the exception?

    6) In the test case, two of the comments have been merged into one giant line. Search for "the mouse down" to see what I mean.

[Xiaomei Ji]

(In reply to comment #10)
> (From update of attachment 31969 [details])
> Six thoughts:
> 
>     1) Please re-test this after http://trac.webkit.org/changeset/45945 -- this
> might have been affected by that bug.

The bug (different  behavior in Windows) still present after the above change.
But the cause probably is different now, I need to re-debug to check the cause.

> 
>     2) #if !PLATFORM(WIN) is the way to do this, not an #if followed by an
> #else.

Will change.

> 
>     3) Does this really not affect the results of any other regression tests?
Mac runs OK for sure. I need to look again at Windows.

> 
>     4) We want the test to test behavior on all platforms, expecting one result
> on Windows and another on other platforms.

Got it. Ran the test in 2 platforms (and got 2 different results), but did not submit it in Mac due to the code change only apply to Widows.

> 
>     5) Is it really true that Windows is the exception? Maybe Mac is the
> exception?

I do not know. Being reported and tested in Windows, so changed only in Windows for safe.

> 
>     6) In the test case, two of the comments have been merged into one giant
> line. Search for "the mouse down" to see what I mean.

Will change.

Thanks,
Xiaomei

[Ojan Vafai]

Created attachment 51110 [details]
Patch

[Peter Kasting]

In response to comment 10 point 5: a quick test on a Linux box confirms that, at least in the environment I checked, selections are directional as on Windows (which is also what the bug title implies); this also makes more intuitive sense to me.

So if I were writing the patch and picking a default behavior, I'd make the Mac behavior the exception rather than the default.

[David Levin]

Comment on attachment 51110 [details]
Patch

Nothing huge but some changes may be significant so r- for one more round.


> diff --git a/LayoutTests/editing/selection/5195166-1.html b/LayoutTests/editing/selection/5195166-1.html

Since you're changing the image result, would you consider making this a dumpAsText test? (I think it already tests everything needed without any changes.)

The plus side is that you won't have to generate the mac result which is missing (but must be incorrect after this change.)


> diff --git a/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expected.txt b/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expected.txt

> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode: [object Text](ef) focusOffset: 1 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode: [object Text](ef) focusOffset: 2 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode: [object Text]( ) focusOffset: 1 isCollapsed: false]
> +PASS Selection is [anchorNode: [object Text](bc) anchorOffset: 0 focusNode: [object HTMLElement](null) focusOffset: 4 isCollapsed: false]

From http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/extend-after-mouse-selection.html
  " // Seleciton goes from the end of "ef" to the first BR element in the contentEditable region."

Is the text of the test incorrect because it refers to "ef"? (If you change this line, please fix this typo "Seleciton" as well.)


   "assertSelectionAt(endTarget.firstChild, 2, document.getElementById('test'), 3);"

I also don't understand how the assert in the test passes.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Ojan Vafai  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        mouse-based selections are always directional on Windows/Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=25195
> +
> +        Change m_lastChangeWasHorizontalExtension to m_isDirectional
> +        and make m_isDirectional always be true for Windows/Linux.
> +
> +        * editing/SelectionController.cpp:
> +        (WebCore::SelectionController::SelectionController):
> +        (WebCore::SelectionController::setSelection):
> +        (WebCore::SelectionController::setIsDirectional):
> +        (WebCore::SelectionController::willBeModified):
> +        When double-clicking the base/extent will be in the middle

When double-clicking,
(Added ,)

> +        of the seleciton instead of the start/end of it. Changed to

typo: seleciton

> +void SelectionController::setIsDirectional(bool isDirectional)
> +{
> +    Settings* settings = m_frame ? m_frame->settings() : 0;
> +    if (settings && settings->editingBehavior() == EditingMacBehavior)
> +        m_isDirectional = isDirectional;
> +    else
> +        m_isDirectional = true;

How do you feel about this?
        m_isDirectional = !settings || (settings->editingBehavior() == EditingMacBehavior && isDirectional);

[Ojan Vafai]

Created attachment 51663 [details]
Patch

[Ojan Vafai]

Comment on attachment 51663 [details]
Patch

Implemented all of Dave's comments.

> The plus side is that you won't have to generate the mac result which is
> missing (but must be incorrect after this change.)

Hah. Oops. I try to remember to run the pixel tests. It's harder when so many of them are broken on ToT.

> > diff --git a/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expected.txt b/LayoutTests/platform/win/editing/selection/extend-after-mouse-selection-expected.txt
> Is the text of the test incorrect because it refers to "ef"? (If you change
> this line, please fix this typo "Seleciton" as well.)
> I also don't understand how the assert in the test passes.

Ugh. Sorry, I somehow left the modifications to this test out of the patch. Thanks for being thorough.

[David Levin]

Comment on attachment 51663 [details]
Patch


> diff --git a/LayoutTests/editing/selection/extend-after-mouse-selection.html b/LayoutTests/editing/selection/extend-after-mouse-selection.html
> +
> +    // On Win/Linux the anchor is be fixed after the mouse-selection and never changes.
> +    // On Mac, the first character-granularity selection after a mouse-selection resets the anchor/focus.
> +    if (onMacPlatform)
> +      assertSelectionAt(endTarget.firstChild, 2, startTarget.previousSibling, 1);
> +    else
> +      assertSelectionAt(startTarget.firstChild, 0, endTarget.firstChild, 1);

> +    if (onMacPlatform)
> +      assertSelectionAt(endTarget.firstChild, 2, startTarget.firstChild, 0);
> +    else
> +      assertSelectionAt(startTarget.firstChild, 0, endTarget.firstChild, 2);
> +

Style for the js test is lax, but it bothers me that the 2 space indent used in the new code is inconsistent with the rest of the file. Please consider adjusting this to be 4 spaces.

[Ojan Vafai]

Committed r56567: <http://trac.webkit.org/changeset/56567>

[Gustavo Noronha (kov)]

This change regresses editing/selection/shrink-selection-after-shift-pagedown.html on Qt, and the new  LayoutTests/editing/selection/extend-after-mouse-selection.html fails on GTK+ (because we currently default to Mac editing behavior - we likely need one of our own).