417418 - Right clicking on the empty spaces to the right of paragraphs selects the whole paragraph

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martin Kou]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

As title.

Reproducible: Always

Steps to Reproduce:
1. Write a simple paragraph in the editor.
2. Move your mouse cursor to the empty spaces to the right of the paragraph.
3. Right click your mouse.
Actual Results:  
The whole paragraph is selected.

Expected Results:  
The right click should move the caret to a spot close to the mouse, but it shouldn't select the whole paragraph. The select paragraph behavior surprises the user since it is different to what he sees from everyday word processors (Microsoft Word, OO.o Writer, etc.) and the editor component of other browsers.

This issue causes bug 440 in FCKeditor:
https://dev.fckeditor.net/ticket/440

Comment 1

[Martin Kou]

Attachment: Simple test case with an editing area and two simple paragraphs prepared.

Comment 2

[Wayne Mery (:wsmwk)]

it is odd. not finding a dupe

Comment 3

[Wayne Mery (:wsmwk)]

it's as if right click is triggering triple click

Comment 4

[paultt]

confirmed
same thing also in GNU/Linux environment, version 3.0.4

User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US;
	rv:1.9.1.9) Gecko/20100515 Lightning/1.0b1 Icedove/3.0.4
	ThunderBrowse/3.2.8.1

and in newer version for windows xp too (that one i could test)

@comment 3:
i think is not triggering triple click, as right clicking correctly popup the popup menu, but it also select all text :(
(didn't look at the code yet)

Comment 5

[(no longer active)]

I can't reproduce.  What are the exact steps to reproduce?

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to comment #5)
> I can't reproduce.  What are the exact steps to reproduce?

in attachment 303209 [details]
- right+click in the whitespace to the right of either paragraph, paragraph is selected
- versus right+click below, or in, either paragraph, no text is selected

Comment 7

[(no longer active)]

Which version of Firefox are you using?  Can you try a nightly build?

Comment 8

[Wayne Mery (:wsmwk)]

I tested with recent build - Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a5pre) Gecko/20100531 Minefield/3.7a5pre

Comment 9

[(no longer active)]

Hmm, OK, this seems to be Windows-only (I was testing on a Mac).

Peter, do you happen to know what code is responsible for this behavior?

Comment 10

[paultt]

it's also gnu/linux related
i tried in linux and in windows, same behaviour
anyway a friend of mine told me he was unable to reproduce
(me and him without any extension installed)

could it be something related to old configurations left around?

Comment 11

[(no longer active)]

No, it doesn't happen on Mac, but happens with a new profile on Windows.

Comment 12

[paultt]

it happens on windows _and_ on linux, and with "compose messages in html format" checked in 'composition & addressing' section of account's options
i'm looking into the code

Comment 14

[arno renevier]

This happens because editor selects clicked element in case of a double click or a "contextclick". And context click is determined as is in editor event listener:
- Ctrl+click on mac
- right click otherwise.
http://hg.mozilla.org/mozilla-central/file/a4d88ea926ec/editor/libeditor/html/nsHTMLEditorEventListener.cpp#l137
This explains then why the bug does not appear on mac. But Ctrl+click or double clicking should exhibit the behaviour we see on other platforms.

This piece of code seems to have been added in bug #112181

What would be the drawbacks of not selecting anything in case of context click (whatever that means) ?

Comment 16

[(no longer active)]

(In reply to comment #14)
> This happens because editor selects clicked element in case of a double
> click or a "contextclick". And context click is determined as is in editor
> event listener:
> - Ctrl+click on mac
> - right click otherwise.
> http://hg.mozilla.org/mozilla-central/file/a4d88ea926ec/editor/libeditor/
> html/nsHTMLEditorEventListener.cpp#l137
> This explains then why the bug does not appear on mac. But Ctrl+click or
> double clicking should exhibit the behaviour we see on other platforms.
> 
> This piece of code seems to have been added in bug #112181
> 
> What would be the drawbacks of not selecting anything in case of context
> click (whatever that means) ?

This seems quite stupid to me.  It might have been something needed by the old Composer back in the day, but it's definitely not something that we want in Firefox though.  That said, I'm also interested to see what roc and bz think about this.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If I open up OpenOffice Writer and right-click on an image, the image is selected.

I think we should treat the context click just like a single click for the purposes of moving the caret and selecting content.

Comment 18

[arno renevier]

Attachment: treat context click as single click

Here is a patch proposal, which removes context click handling in nsHTMLEditorEventListener.
Unfortunately, I could not write some mochitest because context click created with createEvent, does not select the paragraph (from what I understand, that's because PresShell::HandleEvent is not called and then uiEvent->GetRangeParent

Comment 19

[Boris Zbarsky [:bzbarsky]]

> because context click created with createEvent

Don't use createEvent, then.  Use synthesizeMouse() from EventUtils.js, which goes through the full presshell event dispatch.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Or most simply use synthesizeMouseAtCenter if you don't care about click location.

Comment 21

[arno renevier]

With synthetizeMouse, event is dispatched to docshell, but no any more to editor listener.

Comment 22

[Boris Zbarsky [:bzbarsky]]

That's quite odd.  synthesizeMouse should act the same as actual mouse clicks...

Comment 23

[arno renevier]

Attachment: patch with test

For some reason, synthesizeMouse works correctly when called in a timeout in load listener.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Does it work if you waitForFocus ?

Comment 25

[arno renevier]

Attachment: patch using waitForFocus in test

yes, it also works.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does right-clicking on an image still select it?

If so, can you add a test for that?

Comment 27

[arno renevier]

(In reply to comment #26)
> Does right-clicking on an image still select it?
> 

No it's not. Should that be ? If so, should that be on Ctrl-click on mac ?

Comment 28

[(no longer active)]

Comment on attachment 546186 [details] [diff] [review]
patch using waitForFocus in test

I don't think this is what we want to do.  Here's what we should do, I think:

1. Take out that XP_MACOSX #ifdef code (to make us have the same behavior for double-tapping on Mac for example).
2. I think this check should be reversed <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditorEventListener.cpp#250> to only call SelectElement for some types of elements.  Images are the only elements for which we should do this for now, I think.

Comment 29

[arno renevier]

Attachment: patch

I'm not sure I understand. Is attached patch implementing what you are saying ?

Comment 30

[(no longer active)]

Comment on attachment 546224 [details] [diff] [review]
patch

Yes, indeed, this is what I'm talking about.


>diff -r ea0c2d6a0c3d editor/libeditor/html/tests/test_bug417418.html
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/editor/libeditor/html/tests/test_bug417418.html	Fri Jul 15 23:15:19 2011 +0200
>@@ -0,0 +1,42 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=417418
>+-->
>+<head>
>+  <title>Test for Bug 417418</title>
>+  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=417418">Mozilla Bug 417418</a>
>+<div id="display" contenteditable="true">
>+<p id="coin">first paragraph</p>
>+<p>second paragraph</p>
>+</div>
>+<div id="content" style="display: none">
>+
>+</div>
>+<pre id="test">
>+<script class="testbody" type="text/javascript">
>+
>+/** Test for Bug 417418 **/
>+SimpleTest.waitForExplicitFinish();
>+SimpleTest.waitForFocus(runTest);
>+
>+function runTest() {
>+    var target = document.getElementById('display');
>+    var evt = {type: 'mousedown', button: 2};
>+
>+    synthesizeMouseAtCenter(target, evt);

This is probably fine, but I'd feel a lot more comfortable if you click on the far right side of the div.

Can you also add a test to make sure that the same thing happens for a regular click too?  You should just reset the selection (by calling removeAllRanges) between the two tests.

r=me with that.  Thanks a lot for your patch!  :-)

Comment 31

[arno renevier]

Attachment: updated test

updated patch to test single click, and to also test click on image.

Comment 32

[(no longer active)]

Comment on attachment 546288 [details] [diff] [review]
updated test

Thanks a lot!

Comment 33

[(no longer active)]

Landed this on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/65e0d40a57b7

Comment 34

[(no longer active)]

I backed this patch out because the newly added test fails.  See the example log:

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1311016279.1311019114.21393.gz

arno, can you please look into the failure?  Thanks!

Comment 35

[arno renevier]

The test runs correctly on my machine. I have no idea how to reproduce it.

Comment 36

[(no longer active)]

I could reproduce it locally.  My suggestion of removing all of the ranges from the selection was bad, I relanded with a fix:

http://hg.mozilla.org/integration/mozilla-inbound/rev/418854805df8

Comment 37

[(no longer active)]

This was never marked as FIXED for some reason...

http://hg.mozilla.org/mozilla-central/rev/418854805df8