Closed Bug 673315 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Selection, defect)

All
Linux
defect
Not set
major

Tracking

()

RESOLVED WORKSFORME
mozilla9

People

(Reporter: alice0775, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

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
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).
(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).
Looks like the function can be enabled by clipboard.autocopy. However, I cannot enable this feature on Windows, it's odd...
Okay, I see the cause. But I'm not sure why the patch caused this, I'll investigate it more.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
Summary: middlemouse paste does not work properly → clipboard.autocopy does not work properly, selected text is no longer available in clipboard
Severity: normal → major
Summary: clipboard.autocopy does not work properly, selected text is no longer available in clipboard → clipboard.autocopy does not work properly, selected text is no longer available in clipboard (or rather in PRIMARY)
I see this in Thunderbird trunk (Shredder) too.

  Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110722 Thunderbird/8.0a1
(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.
Hardware: x86 → All
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.
Attachment #547870 - Flags: review?(Olli.Pettay)
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.
Attachment #547872 - Flags: review?(Olli.Pettay)
Attachment #547872 - Attachment is obsolete: true
Attachment #547872 - Flags: review?(Olli.Pettay)
Attachment #547876 - Flags: review?(Olli.Pettay)
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?
(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...
Attached patch Patch part.2 approach #1 (obsolete) — Splinter Review
Attachment #547876 - Attachment is obsolete: true
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547976 - Flags: review?(Olli.Pettay)
Attached patch Patch part.2 approach #2 (obsolete) — Splinter Review
This is simplest, but I like #1 better. Please choose from these two (or, is there another approach?).
Attachment #547978 - Flags: review?(Olli.Pettay)
Attached patch Patch part.2 approach #1 (obsolete) — Splinter Review
This is better than previous #1 patch. If we add MOUSEUP_REASON at runtime, (aReason & DRAG_ABORTED_REASON) doesn't hit to MOUSEUP_REASON.
Attachment #547976 - Attachment is obsolete: true
Attachment #547976 - Flags: review?(Olli.Pettay)
Attachment #547981 - Flags: review?(Olli.Pettay)
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.
Attachment #547978 - Flags: review?(Olli.Pettay) → review-
Attachment #547870 - Flags: review?(Olli.Pettay) → review+
(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 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.
Attachment #547876 - Attachment is obsolete: false
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547978 - Attachment is obsolete: true
Attachment #547981 - Attachment is obsolete: true
Attachment #547981 - Flags: review?(Olli.Pettay)
Attachment #547876 - Attachment is obsolete: true
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547999 - Flags: review?(Olli.Pettay)
Attachment #547999 - Flags: review?(Olli.Pettay) → review+
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
Attachment #547999 - Flags: superreview?(roc)
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.
Flags: in-testsuite+
Whiteboard: part.1 landed, part.2 needs sr
Target Milestone: --- → mozilla8
(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.
Attachment #547999 - Flags: superreview?(roc) → superreview+
Depends on: 675614
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Whiteboard: part.1 landed, part.2 needs sr
Target Milestone: mozilla8 → mozilla9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: