Last Comment Bug 417418 - Right clicking on the empty spaces to the right of paragraphs selects the whole paragraph
: Right clicking on the empty spaces to the right of paragraphs selects the who...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla8
Assigned To: arno renevier
:
Mentors:
: 657822 671398 (view as bug list)
Depends on: 1163640
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-13 22:02 PST by Martin Kou
Modified: 2015-05-11 08:26 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple test case with an editing area and two simple paragraphs prepared. (843 bytes, text/html)
2008-02-13 22:03 PST, Martin Kou
no flags Details
treat context click as single click (6.66 KB, patch)
2011-07-15 05:49 PDT, arno renevier
no flags Details | Diff | Review
patch with test (8.75 KB, patch)
2011-07-15 09:53 PDT, arno renevier
no flags Details | Diff | Review
patch using waitForFocus in test (8.70 KB, patch)
2011-07-15 10:19 PDT, arno renevier
ehsan: review-
Details | Diff | Review
patch (4.81 KB, patch)
2011-07-15 14:18 PDT, arno renevier
ehsan: review+
Details | Diff | Review
updated test (5.86 KB, patch)
2011-07-16 00:11 PDT, arno renevier
ehsan: review+
Details | Diff | Review

Description Martin Kou 2008-02-13 22:02:07 PST
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 2008-02-13 22:03:24 PST
Created attachment 303209 [details]
Simple test case with an editing area and two simple paragraphs prepared.
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2008-07-01 06:29:27 PDT
it is odd. not finding a dupe
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2008-09-15 04:58:37 PDT
it's as if right click is triggering triple click
Comment 4 paultt 2010-06-25 04:15:36 PDT
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 :Ehsan Akhgari (out sick) 2010-06-25 13:38:17 PDT
I can't reproduce.  What are the exact steps to reproduce?
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2010-06-27 04:25:24 PDT
(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 :Ehsan Akhgari (out sick) 2010-06-27 10:13:16 PDT
Which version of Firefox are you using?  Can you try a nightly build?
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2010-06-27 17:53:15 PDT
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 :Ehsan Akhgari (out sick) 2010-06-27 18:27:15 PDT
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 2010-06-28 00:01:22 PDT
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 :Ehsan Akhgari (out sick) 2010-06-28 15:31:01 PDT
No, it doesn't happen on Mac, but happens with a new profile on Windows.
Comment 12 paultt 2010-08-25 07:41:55 PDT
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 13 arno renevier 2011-07-14 12:18:12 PDT
*** Bug 657822 has been marked as a duplicate of this bug. ***
Comment 14 arno renevier 2011-07-14 12:32:36 PDT
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 15 arno renevier 2011-07-14 12:33:06 PDT
*** Bug 671398 has been marked as a duplicate of this bug. ***
Comment 16 :Ehsan Akhgari (out sick) 2011-07-14 16:00:40 PDT
(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) (Exited; email my personal email if necessary) 2011-07-14 18:36:12 PDT
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 2011-07-15 05:49:02 PDT
Created attachment 546133 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2011-07-15 06:18:07 PDT
> 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 [:bz] (Out June 25-July 6) 2011-07-15 06:18:34 PDT
Or most simply use synthesizeMouseAtCenter if you don't care about click location.
Comment 21 arno renevier 2011-07-15 07:04:55 PDT
With synthetizeMouse, event is dispatched to docshell, but no any more to editor listener.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-15 07:16:49 PDT
That's quite odd.  synthesizeMouse should act the same as actual mouse clicks...
Comment 23 arno renevier 2011-07-15 09:53:53 PDT
Created attachment 546181 [details] [diff] [review]
patch with test

For some reason, synthesizeMouse works correctly when called in a timeout in load listener.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-15 09:59:04 PDT
Does it work if you waitForFocus ?
Comment 25 arno renevier 2011-07-15 10:19:46 PDT
Created attachment 546186 [details] [diff] [review]
patch using waitForFocus in test

yes, it also works.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-15 13:50:44 PDT
Does right-clicking on an image still select it?

If so, can you add a test for that?
Comment 27 arno renevier 2011-07-15 13:54:32 PDT
(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 :Ehsan Akhgari (out sick) 2011-07-15 13:55:24 PDT
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 2011-07-15 14:18:54 PDT
Created attachment 546224 [details] [diff] [review]
patch

I'm not sure I understand. Is attached patch implementing what you are saying ?
Comment 30 :Ehsan Akhgari (out sick) 2011-07-15 15:21:08 PDT
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 2011-07-16 00:11:18 PDT
Created attachment 546288 [details] [diff] [review]
updated test

updated patch to test single click, and to also test click on image.
Comment 32 :Ehsan Akhgari (out sick) 2011-07-18 11:41:53 PDT
Comment on attachment 546288 [details] [diff] [review]
updated test

Thanks a lot!
Comment 33 :Ehsan Akhgari (out sick) 2011-07-18 11:44:24 PDT
Landed this on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/65e0d40a57b7
Comment 34 :Ehsan Akhgari (out sick) 2011-07-18 13:34:01 PDT
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 2011-07-18 14:05:22 PDT
The test runs correctly on my machine. I have no idea how to reproduce it.
Comment 36 :Ehsan Akhgari (out sick) 2011-07-18 15:50:36 PDT
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 :Ehsan Akhgari (out sick) 2011-07-28 15:43:59 PDT
This was never marked as FIXED for some reason...

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

Note You need to log in before you can comment on or make changes to this bug.