673315 - clipboard.autocopy does not work properly, selected text is no longer available in clipboard (or rather in PRIMARY)

Status: RESOLVED WORKSFORME

(Core :: DOM: Selection, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/6df31af4cca6
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110721 Firefox/8.0a1 ID:20110721030828

middlemouse paste is broken.
See http://forums.mozillazine.org/viewtopic.php?p=11048651#p11048651

Reproducible: Always

Steps to Reproduce:

1. Start Firefox with clean profile.
2. Open any page
3. Select text
4. Middle click on Search Bar

Actual Results: 
  Nothing happens or previous copied text is pasted

Expected Results: 
  Selected text should be pasted

Regression window(cahed m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/5c7a49dfa7f7
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110720 Firefox/8.0a1 ID:20110720133756
Fails:
http://hg.mozilla.org/mozilla-central/rev/be5295604fd0
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110720 Firefox/8.0a1 ID:20110720173300
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c7a49dfa7f7&tochange=be5295604fd0

Regressed by:
be5295604fd0	Masayuki Nakano — Bug 671319 Should abort drag for selection at resetting capture r=smaug

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Is the behavior different on Windows and Linux? I cannot copy by the selected text by middle click on Windows (i.e., text in clipboard already is always pasted).

Comment 2

[Alice0775 White]

(In reply to comment #1)
> Is the behavior different on Windows and Linux? I cannot copy by the
> selected text by middle click on Windows (i.e., text in clipboard already is
> always pasted).
Yes,It is different between Windows and Linux(X Window System).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like the function can be enabled by clipboard.autocopy. However, I cannot enable this feature on Windows, it's odd...

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I see the cause. But I'm not sure why the patch caused this, I'll investigate it more.

Comment 6

[Thomas Ahlblom]

The subject of this bug is currently slightly misleading.

The problem is not pasting. If you select something from another program there is no problem to paste it, using the middle mouse button, into the latest nightly.

But if you select something with the mouse in the latest nightly it doesn't get copied to the copy/paste buffer and you can't paste it, neither into the latest nightly nor into another program.

Comment 7

[Mahesh Asolkar]

I see this in Thunderbird trunk (Shredder) too.

  Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110722 Thunderbird/8.0a1

Comment 8

[Thomas Jones]

(In reply to comment #1)
> Is the behavior different on Windows and Linux? I cannot copy by the
> selected text by middle click on Windows (i.e., text in clipboard already is
> always pasted).

Just to clarify what's going on here, X has two clipboards (well technically three, but nobody uses one of them for anything) PRIMARY, and CLIPBOARD. The bug is that selected text is not going into PRIMARY as it ought to.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.1 selection change reason must be MOUSEUP_REASON when dragging for selection finishes normally

There are two bugs, this patch fixes the first one.

This is a simple mistake. I forgot to change nsFrame::HandleRelease(). By the bug 671319 change, we must call nsFrameSelection::SetMouseDownState(PR_FALSE) before nsIPresShell::SetCapturingContent() in nsFrame::HandleRelease(). If we do so, the selection change reason becomes MOUSEUP_REASON. Otherwise (i.e., currently), nsFrameSelection thinks the drag is aborted rather than finished normally. Then, the selection change reason is NO_REASON.

In PresShell::HandleEventInternal(), we're releasing capturing content always.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7097
So, we don't need to release it in nsFrame::HandleRelease(), therefore, this patch just removes it.

And the nsFrame::HandleRelease()'s only one caller at non-mouseup reason is nsSliderFrame::HandleEvent() when it's called for NS_MOUSE_EXIT_SYNTH. This needs to call nsIPresShell::SetCapturingContent(nsnull, 0) manually. However, I think that this is wrong behavior as I wrote the comment in this patch. I'll look for such bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 add new selection reason for drag for selection aborted

The other cause is fixed by this patch.

Now, we're using NO_REASON when aborted a drag for selection. However, we should use new reason at that time. For example,
> data:text/html,<script type="text/javascript">var count = 0;</script><input value="aaaaaaaaaaaaaaaaaaaaaaa" onmousedown="count=0;" onmousemove="if (count++ == 50) document.releaseCapture();" onselect="alert('onselect fired');">

load this url, and mousedown in the <input> and select a character and release it. Then, you could see an alert dialog if you applied the part.1 patch.

However, if you continued to mousemove, the drag would be aborted automatically but you wouldn't see the alert dialog at that time.

I define a new reason DRAG_ABORTED_REASON. And I change each listeners which are checking the reason.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 add new selection reason for drag for selection aborted

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 547876 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

This is becoming very complex. Is there really no other way to fix this problem?
Or is there some other, less regression risky way to fix bug 671319?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #13)
> This is becoming very complex. Is there really no other way to fix this
> problem?

I think that if we can use MOUSEUP_REASON in nsFrameSelection::AbortDragForSelection(), there is no problem. However, it's very strange by the name. If we don't mind this, it's most simple fix and not risky because some other points are calling nsFrameSelection::SetMouseDownState(PR_FALSE) at non-mouseup situation. For example:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#732
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6467
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1124

I'm not sure whether there are some cases actually to need to distinguish MOUSEUP_REASON and DRAG_ABORTED_REASON. My another idea is, we can define DRAG_ABORTED_REASON as 0x24. Then, (aReason & MOUSEUP_REASON) hits DRAG_ABORTED_REASON too.

> Or is there some other, less regression risky way to fix bug 671319?

This cause isn't bug 671319 actually, I think it's just a trigger. Before bug 552707, we didn't abort dragging for selection when mouse capture is released forcibly. By the behavior, I can see some strange behavior -- I have released mouse button but I'm still dragging. So, the older versions didn't think that dragging is never aborted. This wrong logic makes this bug (only part.2) with current code. Therefore, I believe that bug 671319's patch used a right approach.

# and I realized that I shouldn't change nsHTMLObjectResizer. It's MOUSEDOWN_REASON, not MOUSEUP_REASON...

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 approach #1

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 approach #2

This is simplest, but I like #1 better. Please choose from these two (or, is there another approach?).

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 approach #1

This is better than previous #1 patch. If we add MOUSEUP_REASON at runtime, (aReason & DRAG_ABORTED_REASON) doesn't hit to MOUSEUP_REASON.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 547978 [details] [diff] [review]
Patch part.2 approach #2

The problem with this is that AbortDragForSelection() is called also in
nsIPresShell::SetCapturingContent which can be called while mouse is still down.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #18)
> The problem with this is that AbortDragForSelection() is called also in
> nsIPresShell::SetCapturingContent which can be called while mouse is still
> down.

Of course, yes. Do you think the dragging for selection shouldn't be aborted by releasing the mouse capture? Or, do you think we should deny the releasing mouse capture during dragging for selection?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 547876 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

I and smaug discussed the issues on IRC.

I'm re-requesting review to smaug again.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch part.2 add new selection reason for drag for selection aborted

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 547999 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

oops, this needs sr for interface change.

But I'm not sure who is a good super reviewer for nsISelectionListener. Smaug who is an expert of event handling was already r+ for this patch. Therefore, I'll request sr to roc.

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/base/public/nsISelectionListener.idl&rev=HEAD&mark=1.4
http://hg.mozilla.org/mozilla-central/filelog/5d1799e16fe9/content/base/public/nsISelectionListener.idl

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

landed part.1 first:
http://hg.mozilla.org/mozilla-central/rev/de43fbe04808

You cannot see this bug by this landing, however, don't close this bug. The part.2 bug shows by part.1 fix.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> You cannot see this bug by this landing, however, don't close this bug. The
> part.2 bug shows by part.1 fix.

It means that you can still see this bug if drag is aborted by something. It'll be fixed by part.2.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The regression causes are backedout temporarily. But they will be back again soon for mozilla9. See bug 675865.

The all works for this bug is already done, therefore, I mark this bug as WORKSFORME temporarily.