Closed Bug 417418 Opened 16 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: m.kou, Assigned: arno)

References

Details

Attachments

(2 files, 4 obsolete files)

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
it is odd. not finding a dupe
Status: UNCONFIRMED → NEW
Ever confirmed: true
it's as if right click is triggering triple click
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)
I can't reproduce.  What are the exact steps to reproduce?
(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
Which version of Firefox are you using?  Can you try a nightly build?
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
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?
Version: unspecified → Trunk
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?
No, it doesn't happen on Mac, but happens with a new profile on Windows.
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
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) ?
(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.
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.
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
> because context click created with createEvent

Don't use createEvent, then.  Use synthesizeMouse() from EventUtils.js, which goes through the full presshell event dispatch.
Or most simply use synthesizeMouseAtCenter if you don't care about click location.
With synthetizeMouse, event is dispatched to docshell, but no any more to editor listener.
That's quite odd.  synthesizeMouse should act the same as actual mouse clicks...
Attached patch patch with test (obsolete) — Splinter Review
For some reason, synthesizeMouse works correctly when called in a timeout in load listener.
Assignee: nobody → arno
Attachment #546133 - Attachment is obsolete: true
Attachment #546181 - Flags: review?(roc)
Does it work if you waitForFocus ?
Attached patch patch using waitForFocus in test (obsolete) — Splinter Review
yes, it also works.
Attachment #546181 - Attachment is obsolete: true
Attachment #546186 - Flags: review?(roc)
Attachment #546181 - Flags: review?(roc)
Does right-clicking on an image still select it?

If so, can you add a test for that?
(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 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.
Attachment #546186 - Flags: review?(roc) → review-
Attached patch patch (obsolete) — Splinter Review
I'm not sure I understand. Is attached patch implementing what you are saying ?
Attachment #546186 - Attachment is obsolete: true
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!  :-)
Attachment #546224 - Flags: review+
Attached patch updated testSplinter Review
updated patch to test single click, and to also test click on image.
Attachment #546224 - Attachment is obsolete: true
Attachment #546288 - Flags: review?(ehsan)
Comment on attachment 546288 [details] [diff] [review]
updated test

Thanks a lot!
Attachment #546288 - Flags: review?(ehsan) → review+
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!
The test runs correctly on my machine. I have no idea how to reproduce it.
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
This was never marked as FIXED for some reason...

http://hg.mozilla.org/mozilla-central/rev/418854805df8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.